Skip to content
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

Merged
merged 6 commits into from
Dec 18, 2023

Conversation

deepyaman
Copy link
Member

@deepyaman deepyaman commented Dec 18, 2023

Description

The current sorting algorithm uses the full line, instead of the package name. For example:

Current:

# Pin problematic traitlets release -  https://github.com/jupyter/notebook/issues/7048
ipython~=8.10; python_version >= '3.8'
jupyterlab_server>=2.11.1, <2.16.0
jupyterlab~=3.0, <3.6.0
jupyter~=1.0
kedro-telemetry>=0.3.1
kedro~=0.19.1
traitlets<5.10.0

Expected:

ipython~=8.10; python_version >= '3.8'
jupyter~=1.0
jupyterlab~=3.0, <3.6.0
jupyterlab_server>=2.11.1, <2.16.0
kedro~=0.19.1
kedro-telemetry>=0.3.1
# Pin problematic traitlets release -  https://github.com/jupyter/notebook/issues/7048
traitlets<5.10.0

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

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

@deepyaman deepyaman changed the title Fix/requirements txt sorting Sort requirements.txt based on package name only Dec 18, 2023
@deepyaman deepyaman marked this pull request as ready for review December 18, 2023 16:05
@SajidAlamQB
Copy link
Contributor

SajidAlamQB commented Dec 18, 2023

Initially, I removed the sort_requirements function, but it seems the other starters also rely on it, so will leave it as a wrapper.

You would need to update all the post_gen_project.py in the relevant starters.

Copy link
Contributor

@SajidAlamQB SajidAlamQB left a 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. 👍

@deepyaman
Copy link
Member Author

Initially, I removed the sort_requirements function, but it seems the other starters also rely on it, so will leave it as a wrapper.

You would need to update all the post_gen_project.py in the relevant starters.

Yeah, and would need to do that before updating the code in main. But this works with more minimal changes. :)

@deepyaman deepyaman enabled auto-merge (squash) December 18, 2023 17:11
@deepyaman deepyaman self-assigned this Dec 18, 2023
Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch!

@deepyaman deepyaman merged commit 52d5189 into main Dec 18, 2023
30 checks passed
@deepyaman deepyaman deleted the fix/requirements-txt-sorting branch December 18, 2023 17:14
@@ -28,6 +28,7 @@ dependencies = [
"omegaconf>=2.1.1",
"parse>=1.19.0",
"pluggy>=1.0",
"pre-commit-hooks",
Copy link
Member

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.

Copy link
Member Author

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).

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member Author

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.

AhdraMeraliQB pushed a commit that referenced this pull request Dec 19, 2023
* Sort `requirements.txt` based on package name only

Signed-off-by: Deepyaman Datta <[email protected]>

* Remove now-unused custom `requirements.txt` sorter

Signed-off-by: Deepyaman Datta <[email protected]>

* Ruff format kedro/templates/project/hooks/utils.py

Signed-off-by: Deepyaman Datta <[email protected]>

* Pass the right argument to `fix_requirements` call

Signed-off-by: Deepyaman Datta <[email protected]>

* Add `sort_requirements` until starters are updated

Signed-off-by: Deepyaman Datta <[email protected]>

* Wrap lib call in existing method for compatibility

Signed-off-by: Deepyaman Datta <[email protected]>

---------

Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>
AhdraMeraliQB added a commit that referenced this pull request Dec 20, 2023
* unique tool entires and QoL for pyproject.toml

Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* revert unique entries change

Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Update cli.py template (#3428)

Signed-off-by: Ahdra Merali <[email protected]>

* Add logging about not using async for Sequential and Parallel runners (#3424)

Signed-off-by: Merel Theisen <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Don't include requirements only needed for example pipeline (#3425)

* remove example pipeline requirements

Signed-off-by: Sajid Alam <[email protected]>

* lint

Signed-off-by: Sajid Alam <[email protected]>

* simplify amending kedro[...] lines

Signed-off-by: Sajid Alam <[email protected]>

* keep the version for datasets

Signed-off-by: Sajid Alam <[email protected]>

* lint

Signed-off-by: Sajid Alam <[email protected]>

---------

Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Sort `requirements.txt` based on package name only (#3436)

* Sort `requirements.txt` based on package name only

Signed-off-by: Deepyaman Datta <[email protected]>

* Remove now-unused custom `requirements.txt` sorter

Signed-off-by: Deepyaman Datta <[email protected]>

* Ruff format kedro/templates/project/hooks/utils.py

Signed-off-by: Deepyaman Datta <[email protected]>

* Pass the right argument to `fix_requirements` call

Signed-off-by: Deepyaman Datta <[email protected]>

* Add `sort_requirements` until starters are updated

Signed-off-by: Deepyaman Datta <[email protected]>

* Wrap lib call in existing method for compatibility

Signed-off-by: Deepyaman Datta <[email protected]>

---------

Signed-off-by: Deepyaman Datta <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>

* Pass tools through as list

Signed-off-by: Ahdra Merali <[email protected]>

* Revert "Pass tools through as list"

This reverts commit f4a4a15.

Signed-off-by: Ahdra Merali <[email protected]>

* Remove duplicates

Signed-off-by: Ahdra Merali <[email protected]>

* Fix for no add-ons selected

Signed-off-by: Ahdra Merali <[email protected]>

* Lint

Signed-off-by: Ahdra Merali <[email protected]>

* Add example as recognised key

Signed-off-by: Ahdra Merali <[email protected]>

* Add example as recognised key pt2

Signed-off-by: Ahdra Merali <[email protected]>

* Add example as recognised key pt3

Signed-off-by: Ahdra Merali <[email protected]>

* Add example as recognised key pt4

Signed-off-by: Ahdra Merali <[email protected]>

* Add example as recognised key pt5

Signed-off-by: Ahdra Merali <[email protected]>

* Add example as recognised key pt6

Signed-off-by: Ahdra Merali <[email protected]>

* Add example as recognised key pt7

Signed-off-by: Ahdra Merali <[email protected]>

* Streamline condition

Signed-off-by: Ahdra Merali <[email protected]>

---------

Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Ahdra Merali <[email protected]>
Signed-off-by: Merel Theisen <[email protected]>
Signed-off-by: Sajid Alam <[email protected]>
Signed-off-by: Deepyaman Datta <[email protected]>
Co-authored-by: Ahdra Merali <[email protected]>
Co-authored-by: Merel Theisen <[email protected]>
Co-authored-by: Deepyaman Datta <[email protected]>
Co-authored-by: Ahdra Merali <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants