[ENH] Shorten package import path from pytorch_forecasting to ptf - #2343
[ENH] Shorten package import path from pytorch_forecasting to ptf#2343harshsomankar123-tech wants to merge 5 commits into
Conversation
…h legacy redirect
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
14390f4 to
1fbc80d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2343 +/- ##
=======================================
Coverage ? 87.46%
=======================================
Files ? 175
Lines ? 10165
Branches ? 0
=======================================
Hits ? 8891
Misses ? 1274
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fkiraly
left a comment
There was a problem hiding this comment.
May I suggest to sequence this as following, for ensuring maximum downwards compatibility:
- instead of actually moving the folder in 1.X, do it the other way round - make
ptfforward topytorch-forecasting. - at 2.0, switch the situation around
Documentation change right now, as planned.
This way we minimize the risk of accidental breakage, since make no major change to folders in an 1.X version.
What do you think, @phoeenniixx?
This PR can be the 2.0 PR, to be parked until the release.
I totally agree with this. This will break a lot of code. |
|
@phoeenniixx @fkiraly Thank you for the review and feedback! Would it make sense to update this PR instead by:
This would avoid the risky folder rename in the 1.x line while still introducing the new import path. Also, regarding @phoeenniixx point, would it make sense to keep the pytorch_forecasting folder permanently, with ptf acting as a forwarding wrapper? I think its a good idea it seems like the safest long-term approach to avoid breaking downstream books, tutorials, and existing code. WDYT???? |
|
The strategy I would suggest is as above:
|
04d5d96 to
5b3cef0
Compare
|
Hi @fkiraly @phoeenniixx , Here's what I changed:
I'd appreciate it if you could take another look whenever you have time. |
…h legacy redirect
5b3cef0 to
e78358c
Compare
|
Thanks, I think as @fkiraly suggested, this will be parked till v2.0 release and will act as a release PR for that... |
Reference Issues/PRs
Fixes #2289.
What does this implement/fix? Explain your changes.
ptfImport Wrapper: Created a lightweightptf/package wrapper containing a custom import redirect hook. Wheneverptfor its sub-modules (e.g.,ptf.models) are imported, it dynamically redirects them to the corresponding module inpytorch_forecasting.pytorch_forecasting/and left all internal codebase/test imports unchanged. This avoids any breaking changes or accidental deployment breakage in the current 1.x release cycle.What should a reviewer concentrate their feedback on?
ptf/__init__.py.Did you add any tests for the change?
Yes, added
tests/test_ptf_import.pywhich explicitly tests the redirection hook to ensure that importing fromptfworks correctly and resolves to the same classes inpytorch_forecasting.PR checklist