-
Notifications
You must be signed in to change notification settings - Fork 905
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sort requirements.txt
based on package name only
#3436
Conversation
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
194e564
to
124685a
Compare
requirements.txt
based on package name only
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu>
You would need to update all the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you @deepyaman. 👍
Yeah, and would need to do that before updating the code in main. But this works with more minimal changes. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch!
@@ -28,6 +28,7 @@ dependencies = [ | |||
"omegaconf>=2.1.1", | |||
"parse>=1.19.0", | |||
"pluggy>=1.0", | |||
"pre-commit-hooks", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not extremely happy about adding a runtime dependency on pre-commit-hooks
though. At least it's unconstrained, but (1) it's unclear to me if the project is meant to be used from its Python API (not that we're using a private function or anything, but I don't see references to it on the READMEs) and (2) this dependency only exists for kedro new
and users are already complaining that we have too many of them, this seems to go in the opposite direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@astrojuanlu If you feel strongly against it, I can just copy the implementation of that particular function. I added this because pre-commit-hooks
is an extremely lightweight library (only dependency is ruamel.yaml
, which itself is very light).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@merelcht what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @astrojuanlu, this sort of dependency isn't core to Kedro as a Framework and so it doesn't belong as a run-time dependency. If anything it should be something like a "dev" dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@merelcht I don't think that's @astrojuanlu's point. I am using pre-commit-hooks
as a runtime dependency here, because the starter requirements get sorted.
The question is whether to vendor the requirements sorting logic or leave this lightweight dependency.
* Sort `requirements.txt` based on package name only Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu> * Remove now-unused custom `requirements.txt` sorter Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu> * Ruff format kedro/templates/project/hooks/utils.py Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu> * Pass the right argument to `fix_requirements` call Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu> * Add `sort_requirements` until starters are updated Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu> * Wrap lib call in existing method for compatibility Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu> --------- Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu> Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com>
* unique tool entires and QoL for pyproject.toml Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com> Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * revert unique entries change Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com> Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Update cli.py template (#3428) Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Add logging about not using async for Sequential and Parallel runners (#3424) Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com> Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Don't include requirements only needed for example pipeline (#3425) * remove example pipeline requirements Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com> * lint Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com> * simplify amending kedro[...] lines Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com> * keep the version for datasets Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com> * lint Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com> --------- Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com> Signed-off-by: Sajid Alam <90610031+SajidAlamQB@users.noreply.github.com> Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Sort `requirements.txt` based on package name only (#3436) * Sort `requirements.txt` based on package name only Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu> * Remove now-unused custom `requirements.txt` sorter Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu> * Ruff format kedro/templates/project/hooks/utils.py Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu> * Pass the right argument to `fix_requirements` call Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu> * Add `sort_requirements` until starters are updated Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu> * Wrap lib call in existing method for compatibility Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu> --------- Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu> Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Pass tools through as list Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Revert "Pass tools through as list" This reverts commit f4a4a15. Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Remove duplicates Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Fix for no add-ons selected Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Lint Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Add example as recognised key Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Add example as recognised key pt2 Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Add example as recognised key pt3 Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Add example as recognised key pt4 Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Add example as recognised key pt5 Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Add example as recognised key pt6 Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Add example as recognised key pt7 Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> * Streamline condition Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> --------- Signed-off-by: Sajid Alam <sajid_alam@mckinsey.com> Signed-off-by: Ahdra Merali <ahdra.merali@quantumblack.com> Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com> Signed-off-by: Sajid Alam <90610031+SajidAlamQB@users.noreply.github.com> Signed-off-by: Deepyaman Datta <deepyaman.datta@utexas.edu> Co-authored-by: Ahdra Merali <90615669+AhdraMeraliQB@users.noreply.github.com> Co-authored-by: Merel Theisen <49397448+merelcht@users.noreply.github.com> Co-authored-by: Deepyaman Datta <deepyaman.datta@utexas.edu> Co-authored-by: Ahdra Merali <ahdra.merali@quantumblack.com>
Description
The current sorting algorithm uses the full line, instead of the package name. For example:
Current:
Expected:
I just noticed it also throws the comments at the top, which takes them out of context.
Development notes
Initially, I removed the
sort_requirements
function, but it seems the other starters also rely on it, so will leave it as a wrapper.Developer Certificate of Origin
We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a
Signed-off-by
line in the commit message. See our wiki for guidance.If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.
Checklist
RELEASE.md
file