Conversation
Deprecates `quantecon.util.notebooks` in 0.12.0 for removal in v1.0, and moves `requests` out of the mandatory dependency set into a `notebooks` extra that is removed with the module. `requests` served exactly one call site in the package. The function has no remaining constituency: no usage across lecture-python.myst, lecture-python-advanced.myst, lecture-python-intro or lecture-python-programming.myst, and the callers that exist are dormant notebooks descending from two 2017-era demos. Its default remote, QuantEcon/QuantEcon.notebooks, was last pushed 2023-07-15. Data still needed moves to QuantEcon/data-lectures. Calling the function emits a DeprecationWarning attributed to the caller's frame. Where `requests` is absent the lazy import raises an ImportError naming the extra. The stale 2016 TODO block, which no longer described the code, is replaced by the deprecation directive. Adds `r.raise_for_status()` as harm reduction for the module's final release: #870 reported that a missing remote path saved GitHub's 404 HTML page under the requested filename and reported success. Verified against the reported reproducer, which now raises HTTPError and writes no file. The other three defects in #870 are left unfixed, the function being scheduled for removal. Part of #880. Supersedes #870, #904 and #905, all closed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
10 tasks
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The updated tests and deprecated helper still contain a few concrete failure modes (optional-dep test gating, teardown robustness, and input handling) that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR deprecates quantecon.util.notebooks.fetch_nb_dependencies ahead of v1.0 removal and moves requests out of QuantEcon.py’s mandatory dependencies by placing it behind a notebooks extra.
Changes:
- Add module/function deprecation directives and a runtime
DeprecationWarning(withstacklevel=2) forfetch_nb_dependencies. - Make
requestsan optional dependency viaquantecon[notebooks], with an actionableImportErrorwhen missing. - Add a regression test asserting the deprecation warning is emitted.
File summaries
| File | Description |
|---|---|
| quantecon/util/notebooks.py | Adds deprecation messaging, optional requests import with helpful error, and raise_for_status() harm reduction. |
| quantecon/util/tests/test_notebooks.py | Suppresses deprecation warnings for existing tests and adds a new test asserting a DeprecationWarning. |
| pyproject.toml | Removes requests from core dependencies and introduces a notebooks optional-dependency group. |
Review details
Suppressed comments (1)
quantecon/util/notebooks.py:101
fetch_nb_dependenciesstill usestype(files) == list, which rejects tuples and other sequences (and can fall through to the dict branch and raiseAttributeError). Since the function remains callable until v1.0, this should be the saferisinstance(..., (list, tuple))check.
)
#-Generate Common Data Structure-#
if type(files) == list:
files = {"" : files}
- Files reviewed: 3/3 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -99,6 +122,8 @@ def fetch_nb_dependencies(files, repo=REPO, raw=RAW, branch=BRANCH, folder=FOLDE | |||
| #-Get file in OS agnostic way using requests-# | |||
| url = "/".join([repo, raw, branch, folder, fl]) | |||
| r = requests.get(url) | |||
|
|
||
| from quantecon.util import fetch_nb_dependencies | ||
| import os | ||
| import pytest |
| @@ -39,3 +41,16 @@ def test_fetch_nb_dependencies_overwrite(self): | |||
|
|
|||
| def teardown_method(self): | |||
| os.remove("test_file.md") | |||
Comment on lines
127
to
128
| with open(fl, "wb") as fl: | ||
| fl.write(r.content) |
|
coverage: 90.89% (+0.006%) from 90.884% — depr/notebooks-utility-880 into main |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Retires
quantecon/util/notebooks.py: deprecated in 0.12.0, removed in v1.0, withrequestsmoved out of the mandatory dependency set at the same time.Why
requestsis a hard runtime dependency of QuantEcon.py that serves exactly one call site —fetch_nb_dependencies— and nothing else in the package imports it. That function has no remaining constituency:lecture-python.myst,lecture-python-advanced.myst,lecture-python-introorlecture-python-programming.mystWald_Friedman,von_neumann)QuantEcon/QuantEcon.notebooks, last pushed 2023-07-15Datasets the lectures depend on belong in QuantEcon/data-lectures, the canonical home for data referenced by stable URL. This is the retirement branch #880 anticipated for
requests: move it behind an extra "or, iffetch_nb_dependencieshas no remaining constituency, fold its retirement into the #786 utility-deprecation conversation". That condition is now tested and met, so this lands in the same release as #833's timer deprecations.Changes
.. deprecated:: 0.12.0directives on the module and the function, replacing the stale 2016 TODO block that no longer described the codeDeprecationWarningat call time,stacklevel=2so it is attributed to the caller's framerequestsmoved fromdependenciesto anotebooksoptional group; the lazy import now raises anImportErrornamingpip install 'quantecon[notebooks]'r.raise_for_status()as harm reduction for the module's final release — see belowfilterwarnings("ignore::DeprecationWarning")On the
raise_for_statusline#870 reported that a missing remote path saved GitHub's 404 HTML page under the requested filename and reported success — a 314 KB HTML document written as your
.csv, withTruein the status list. Since the function keeps working for one more release, it should not corrupt data while it does. Verified against the reported reproducer:The other three defects in #870 (no timeout, the file handle shadowing the loop variable,
type(files) == list) are deliberately left unfixed — the function is scheduled for removal, and #905 and #904 were closed on that basis.Verification
flake8 --select=F401,F405,E231 quantecon: cleangit diff --check: cleannotebooks.pyrequestspath confirmed to raise theImportErrornaming the extraenvironment.ymllistsrequestsandurllib3>=2explicitly, so the test environment keeps them while the module existsFollow-ups
requestsandurllib3come out ofenvironment.ymlwhen the module is removed in v1.0 — tracked separatelysympyhalf of ENH: Slim the mandatory dependency set: sympy and requests #880 landed in MAINT: Remove the sympy dependency #947; this closes therequestshalfPart of #880. Supersedes #870, #904 and #905, all closed.
🤖 Generated with Claude Code