Add a new `SessionConfig` object to allow advanced configuration of the `Downloader` #92

Cadair · 2022-06-09T15:28:22Z

This PR introduces breaking API changes, it will form a 2.0 release of parfive.

fixes #38
fixes #29

This PR is motivated by the fact that we have been slowly growing more and more keyword arguments, environment variables etc at various levels in the class.

This PR cleans up the environment variable handling and keyword arguments to the main Downloader class by making a config object which holds the state for the instance. As well as this some of the more obscure keyword arguments are moved into a SessionConfig object.

Changes in this PR

A new PARFIVE_HIDE_PROGRESS environment variable will disable all progress bars.
The file_progress=, use_aiofiles= and notebook= keyword arguments have been moved into the new SessionConfig object which can be passed to Downloader() as the config= keyword argument.
The headers= keyword argument to Downloader() is now deprecated in favour of passing them as part of the SessionConfig object. They will be removed in a future release.
All environment variables are now evaluated at the time the Downloader class is instantiated (by the new config classes).
The value of the boolean environment variables now matter, they should cast to True.
The timeouts keyword argument has been removed from the run_download() and download() methods and replaced by an option in SessionConfig.
It is now possible to configure the options passed to aiohttp.ClientSession with the aiohttp_session_kwargs= keyword argument to SessionConfig.
It is now possible to configure debug or other levels of logging with SessionConifg where previously it could only be configured with an environment variable.
It is now possible to configure the HTTP chunksize with SessionConfig and the default when not using aiofiles has been increased from 100 bytes to 1024 bytes.
It is now possible to configure the HTTP(S) proxy URLs with SessionConfig as well as the HTTP[S]_PROXY environment variables.
It is now possible to customise all the timeouts supported by aiohttp by passing an aiohttp.ClientTimeout object to SessionConfig.

codecov · 2022-06-09T15:28:37Z

Codecov Report

Merging #92 (106d5c2) into main (c52cbe3) will increase coverage by 0.84%.
The diff coverage is 97.81%.

@@            Coverage Diff             @@
##             main      #92      +/-   ##
==========================================
+ Coverage   92.48%   93.33%   +0.84%     
==========================================
  Files           4        5       +1     
  Lines         466      540      +74     
==========================================
+ Hits          431      504      +73     
- Misses         35       36       +1

Impacted Files	Coverage Δ
parfive/downloader.py	`93.26% <95.34%> (-0.41%)`	⬇️
parfive/config.py	`98.87% <98.87%> (ø)`
parfive/utils.py	`94.54% <100.00%> (+0.15%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c52cbe3...106d5c2. Read the comment docs.

for more information, see https://pre-commit.ci

Cadair · 2022-06-10T10:40:00Z

I am going to ping people who have contributed to see if anyone wants to comment on this PR, and give people a chance to ask for tweaks before I push a major release.

@GitHK @rlaker @nabobalis @dstansby

dstansby · 2022-06-10T11:37:49Z

azure-pipelines.yml

@@ -38,6 +38,9 @@ stages:
          - linux: build_docs
            pytest: false

+          - linux: mypy


mypy has pre-commit hooks if you want to avoid adding another job here: https://github.com/pre-commit/mirrors-mypy

You could have both. It usually helps.

I only didn't put it in the pre-commit because mypy can be a little slow to run (and there wasn't any mention of the pre-commit in mypy's docs).

Also I in general prefer using pre-commit for stuff which autofixes, especially with the pre-commit ci service autofixing now.

dstansby

I got a bit bored reviewing this, sorry, but hopefully what I've left are some very minor but helpful comments...

parfive/config.py

GitHK

The refactoring is looking good.

I would only suggest to use a library more suited to handling env vars than manually parsing them. I've suggested pydantic below.

GitHK · 2022-06-10T12:00:24Z

azure-pipelines.yml

@@ -38,6 +38,9 @@ stages:
          - linux: build_docs
            pytest: false

+          - linux: mypy


You could have both. It usually helps.

parfive/config.py

Cadair · 2022-06-10T12:46:34Z

Thanks for the review @GitHK, are you using the use_aiofiles= kwarg to Downloader? Would removing it cause you trouble?

GitHK · 2022-06-10T12:53:19Z

Thanks for the review @GitHK, are you using the use_aiofiles= kwarg to Downloader? Would removing it cause you trouble?

No issue for me. Go ahead and remove it. We usually do periodic updates and fix these small issues.
It helps a lot when libraries point breaking changes in the changelog, upgrades are much smoother.

Cadair · 2022-06-10T15:03:21Z

@GitHK Thanks! I have pulled it now, I am going to keep headers around for people using previous versions of sunpy.

Cadair · 2022-06-10T15:33:39Z

I have pushed 2.0.0rc1 up to PyPI if people want to test this and open any issues.

parfive/config.py

nabobalis · 2022-06-10T17:44:32Z

docs/conf.py

@@ -36,6 +36,7 @@
    "sphinx.ext.napoleon",
    "sphinx.ext.todo",
    "sphinx.ext.viewcode",
+    "sphinx_autodoc_typehints",  # must be loaded after napoleon


How well does this work?

I have only used it in two small libraries, this one and dkist and so far very happy.

What about if we used this for a bigger library that starts with the letter s?

nabobalis · 2022-06-10T17:49:19Z

Overall:

👍 on pydantic, such a great library.
👍 on the config refactor, makes things much clearer in the main downloader code.
👎 on adding type hints and using black.

Cadair added 2 commits June 9, 2022 15:33

Refactor API simplify config

715e667

Further typehinting

40f23cc

Cadair added 4 commits June 9, 2022 16:45

More docs

5735f91

We need the Literal backport on Python 3.7

8190ab0

Deprecate headers= and use_aiofiles= Downloader kwargs

dcea288

Assorted fixes

77c21cb

Cadair force-pushed the api_refactor branch from 20d0c39 to 77c21cb Compare June 10, 2022 09:52

Cadair and others added 5 commits June 10, 2022 11:05

Allow configuration of log level in SessionConfig

b324dbb

Configure black to 100 lines in the right place

0af5ea1

Bad black config

ebed48b

timeouts cleanup

d332913

[pre-commit.ci] auto fixes from pre-commit.com hooks

14e9179

for more information, see https://pre-commit.ci

Cadair changed the title ~~Unravel the API~~ Add a new SessionConfig object to allow advanced configuration of the Downloader Jun 10, 2022

Cadair marked this pull request as ready for review June 10, 2022 10:36

dstansby reviewed Jun 10, 2022

View reviewed changes

parfive/config.py Outdated Show resolved Hide resolved

parfive/config.py Outdated Show resolved Hide resolved

GitHK reviewed Jun 10, 2022

View reviewed changes

Cadair added 3 commits June 10, 2022 15:59

Make sure that when progress is False we don't draw any progress bars

df80a50

Use pydantic for config validation

b0bbcd7

Fix mypy

91f74d5

Use platform

3698314

Cadair force-pushed the api_refactor branch from b81a2fd to 3698314 Compare June 10, 2022 15:06

More env var docs

106d5c2

Cadair added the breaking A Breaking change label Jun 10, 2022

Cadair merged commit 0664d03 into main Jun 10, 2022

Cadair deleted the api_refactor branch June 10, 2022 15:23

nabobalis reviewed Jun 10, 2022

View reviewed changes

parfive/config.py Show resolved Hide resolved

nabobalis reviewed Jun 10, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new `SessionConfig` object to allow advanced configuration of the `Downloader` #92

Add a new `SessionConfig` object to allow advanced configuration of the `Downloader` #92

Cadair commented Jun 9, 2022 •

edited

Loading

codecov bot commented Jun 9, 2022 •

edited

Loading

Cadair commented Jun 10, 2022

dstansby Jun 10, 2022

GitHK Jun 10, 2022

Cadair Jun 10, 2022

Cadair Jun 10, 2022

dstansby left a comment

GitHK left a comment

GitHK Jun 10, 2022

Cadair commented Jun 10, 2022

GitHK commented Jun 10, 2022

Cadair commented Jun 10, 2022

Cadair commented Jun 10, 2022

nabobalis Jun 10, 2022

Cadair Jun 10, 2022

nabobalis Jun 10, 2022

nabobalis commented Jun 10, 2022 •

edited

Loading

Add a new SessionConfig object to allow advanced configuration of the Downloader #92

Add a new SessionConfig object to allow advanced configuration of the Downloader #92

Conversation

Cadair commented Jun 9, 2022 • edited Loading

Changes in this PR

codecov bot commented Jun 9, 2022 • edited Loading

Codecov Report

Cadair commented Jun 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstansby left a comment

Choose a reason for hiding this comment

GitHK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cadair commented Jun 10, 2022

GitHK commented Jun 10, 2022

Cadair commented Jun 10, 2022

Cadair commented Jun 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabobalis commented Jun 10, 2022 • edited Loading

Add a new `SessionConfig` object to allow advanced configuration of the `Downloader` #92

Add a new `SessionConfig` object to allow advanced configuration of the `Downloader` #92

Cadair commented Jun 9, 2022 •

edited

Loading

codecov bot commented Jun 9, 2022 •

edited

Loading

nabobalis commented Jun 10, 2022 •

edited

Loading