Dependency cleanup: remove pandas, torchvision, and onnx as core dependencies - #2002
coreyjadams wants to merge 4 commits into
Conversation
…es as optional dependencies.
CODEOWNERS review mapCurrent for commit ⏳ @CharlelieLrt — 1 file(s)
⏳ @coreyjadams — 2 file(s)
⏳ @ktangsali — 2 file(s)
⏳ @mnabian — 1 file(s)
⏳ @pzharrington — 12 file(s)
No CODEOWNER
Comment |
|
Fix the pandas-dependent test collection failure before merging. FindingsSummaryRemoves onnx, torchvision, and pandas from the direct core requirements, moves pandas into feature extras, and converts remaining pandas imports to lazy optional imports.
Reviews (1) · Last reviewed commit: "Remove pandas from utils entirely, pushi..." |
| "vtk", | ||
| ] | ||
| datapipes-extras = [ | ||
| "pandas>=2.2.0", |
There was a problem hiding this comment.
Minimal-install test collection breaks
The minimal-install CI jobs install core plus dev, without model-extras or datapipes-extras, then run all of test/. Moving pandas into these extras leaves that environment without pandas, but test/datapipes/healda/test_time_utils.py:21 still imports it unconditionally. Collection therefore fails with ModuleNotFoundError before the tests can run or skip. Change that import to pytest.importorskip("pandas"), or explicitly include pandas in the test dependencies.
| ) | ||
| # Get the day of year as a float: elapsed days since Jan 1 of each date's year. | ||
| dates_arr = np.array(dates, dtype="datetime64") | ||
| days_arr = (dates_arr - dates_arr.astype("datetime64[Y]")) / np.timedelta64(1, "D") |
There was a problem hiding this comment.
Coarse-resolution datetime inputs fail
For an input such as np.array(["2024-02"], dtype="datetime64[M]"), this subtraction produces timedelta64[M]. Dividing it by timedelta64[D] raises TypeError because NumPy cannot convert variable-length months to days. Previously, the pandas-created January 1 timestamps promoted the subtraction to a fixed-duration unit, so this input worked. Cast the year-start array to datetime64[D] before subtraction, and add regression tests for monthly/yearly inputs alongside sub-day and leap-year dates.
| days_arr = (dates_arr - dates_arr.astype("datetime64[Y]")) / np.timedelta64(1, "D") | |
| start_years = dates_arr.astype("datetime64[Y]").astype("datetime64[D]") | |
| days_arr = (dates_arr - start_years) / np.timedelta64(1, "D") |
| days_arr = (np.array(dates, dtype="datetime64") - start_years) / np.timedelta64( | ||
| 1, "D" | ||
| ) | ||
| # Get the day of year as a float: elapsed days since Jan 1 of each date's year. |
There was a problem hiding this comment.
This implementation is not exactly equivalent. Make sure it does not break downstream tasks.
There was a problem hiding this comment.
I added more complexity but Claude thinks it checks out....
ktangsali
left a comment
There was a problem hiding this comment.
LGTM. Good to check about the downstream applications with the changes to insolation (Mohammad's comment)
| "treelib>=1.2.5", | ||
| "numpy>=1.22.4", | ||
| "torch>=2.10.0", | ||
| "torchvision>=0.25.0a0", |
There was a problem hiding this comment.
Goodbye! You will not be missed 😅
There was a problem hiding this comment.
Insolation was fully untested before?? Sheesh
| def test_pandas_timestamps_match_datetime(): | ||
| """pandas Timestamps and a tz-aware DatetimeIndex behave like datetime objects.""" | ||
| pd = pytest.importorskip("pandas") | ||
| ts = [pd.Timestamp("2020-12-31 21:00", tz="US/Eastern")] |
There was a problem hiding this comment.
Out of pure superstition I think we should test a few more trial dates/timezones (or lack thereof) 😅
pzharrington
left a comment
There was a problem hiding this comment.
LGTM as long as the insolation results are identical (already-released models are trained using that quantity)
PhysicsNeMo Pull Request
Onnx and Torchvision are never actually imported anywhere in the repository. They are removed "for free". Onnx is brought back in the
devlist for some tests.pandasis trickier, we used it in utils for this insolation file. I move that calculation to numpy since it is 100% equivalent to datatime work.Description
Checklist
Dependencies
Review Process
All PRs are reviewed by the PhysicsNeMo team before merging.
Depending on which files are changed, GitHub may automatically assign a maintainer for review.
We are also testing AI-based code review tools (e.g., Greptile), which may add automated comments with a confidence score.
This score reflects the AI’s assessment of merge readiness and is not a qualitative judgment of your work, nor is
it an indication that the PR will be accepted / rejected.
AI-generated feedback should be reviewed critically for usefulness.
You are not required to respond to every AI comment, but they are intended to help both authors and reviewers.
Please react to Greptile comments with 👍 or 👎 to provide feedback on their accuracy.