Skip to content

ci: Consolidate requirements into pyproject.toml #7565

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

Closed
wants to merge 26 commits into from

Conversation

CaedenPH
Copy link
Contributor

@CaedenPH CaedenPH commented Oct 23, 2022

Describe your change:

Consolidate the requirements into pyproject.toml
https://pip.pypa.io/en/stable/reference/build-system/pyproject-toml/

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

@algorithms-keeper algorithms-keeper bot added enhancement This PR modified some existing files awaiting reviews This PR is ready to be reviewed labels Oct 23, 2022
@CaedenPH
Copy link
Contributor Author

@cclauss Never used pyproject.toml, so expecting lots of changes

@algorithms-keeper algorithms-keeper bot added the tests are failing Do not merge until tests pass label Oct 23, 2022
@cclauss
Copy link
Member

cclauss commented Oct 23, 2022

We could also move our codespell, mypy, and pyupgrade args from .pre-commit into pyproject.toml.

Please run https://pypi.org/project/validate-pyproject to validate the file and let's add that tool to pre-commit.

@cclauss
Copy link
Member

cclauss commented Oct 23, 2022

We might run into trouble consolidating our pytest args because our GitHub Actions run it two different ways. So that might be difficult to express.

@CaedenPH
Copy link
Contributor Author

We could also move our codespell, mypy, and pyupgrade args from .pre-commit into pyproject.toml.

Please run https://pypi.org/project/validate-pyproject to validate the file and let's add that tool to pre-commit.

How would this be done?

Co-authored-by: Christian Clauss <[email protected]>
@dhruvmanila
Copy link
Member

The only benefit we get in using pyproject.toml is to have a central place for configuration. We don't need the project declaraction, build system, dependencies.

Build system is used to provide a backend like poetry, flit or setuptools to actually build the Python package (wheels). For our use case, requirements.txt is actually the best option instead of declaring it in pyproject.toml. Otherwise, users will be required to install using pip install . which installs the dependencies along with the package (remember this is not a package) and the egg info. Project declaration is not useful as it's used to provide metadata information for PyPi. I guess it was added to declare the dependencies which as mentioned not useful. Adding the authors field is a bit misleading as this is a work of all the contributors.

@cclauss
Copy link
Member

cclauss commented Oct 23, 2022

Let's use maintainers rather than authors... https://peps.python.org/pep-0621/#authors-maintainers

Would we be able to use pip install . or probably better pip install --editable . in our GitHub Actions?

Could pip install git+https://github.com/TheAlgorithms/Python.git be made to work?

@dhruvmanila
Copy link
Member

As I said, this won't work as TheAlgorithms/Python is not a "Python package", you cannot install it as it's not even structured like a standard package. I don't really see why you're wasting time in this.

@cclauss
Copy link
Member

cclauss commented Oct 29, 2022

I fail to understand the logic. pip knows how to install project dependencies from two different file types. Why does it matter whether this is a package or not?

Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

I guess I must've misunderstood something, if it works then it's fine @cclauss

@algorithms-keeper algorithms-keeper bot added awaiting changes A maintainer has requested changes to this PR and removed awaiting reviews This PR is ready to be reviewed labels Oct 30, 2022
Co-authored-by: Dhruv Manilawala <[email protected]>
@algorithms-keeper algorithms-keeper bot added awaiting reviews This PR is ready to be reviewed and removed awaiting changes A maintainer has requested changes to this PR labels Oct 30, 2022
@algorithms-keeper algorithms-keeper bot added the merge conflicts Open a new PR or rebase on the latest commit label Oct 30, 2022
@algorithms-keeper algorithms-keeper bot removed the merge conflicts Open a new PR or rebase on the latest commit label Oct 30, 2022
@CaedenPH CaedenPH requested review from dhruvmanila and cclauss and removed request for dhruvmanila October 30, 2022 13:43
@algorithms-keeper algorithms-keeper bot added awaiting changes A maintainer has requested changes to this PR and removed awaiting reviews This PR is ready to be reviewed labels Oct 30, 2022
@algorithms-keeper algorithms-keeper bot added awaiting reviews This PR is ready to be reviewed and removed awaiting changes A maintainer has requested changes to this PR labels Oct 30, 2022
@CaedenPH CaedenPH requested review from dhruvmanila and removed request for cclauss October 30, 2022 15:52
Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 🎉

@algorithms-keeper algorithms-keeper bot removed the awaiting reviews This PR is ready to be reviewed label Oct 30, 2022
@CaedenPH CaedenPH requested a review from cclauss October 30, 2022 17:25
Copy link
Member

@poyea poyea left a comment

Choose a reason for hiding this comment

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

Thank you!

"requests",
"rich",
"scikit-fuzzy",
"sklearn",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"sklearn",
"scikit-learn",

https://pypi.org/project/scikit-learn/

"sklearn",
"statsmodels",
"sympy",
"tensorflow",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"tensorflow",
# "tensorflow", # Not yet compatible with Python 3.11

@CaedenPH CaedenPH closed this May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This PR modified some existing files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants