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

chore(build): migrate to pyproject.toml for packaging #97

Merged
merged 9 commits into from
Apr 23, 2024
Merged

chore(build): migrate to pyproject.toml for packaging #97

merged 9 commits into from
Apr 23, 2024

Conversation

SauravMaheshkar
Copy link
Contributor

Mirrors work done in:

pyproject.toml is the new standard for packaging python packages, setup.py is now deprecated (first introduced in PEP 518 and later expanded in PEP 517, PEP 621 and PEP 660). This PR proposes to switch from the legacy setup.py method in favour of pyproject.toml. This would lead to a overall better project structure because all requirements would be managed within a single pyproject.toml file instead of 4 separate text files. It also allows for dynamic version allocation (reads automatically from /__init__.py).

Changes proposed by this PR can be summarized as follows :-

  • Delete setup.py in favour of pyproject.toml for packaging and update the test.sh script appropriately.
  • Update Github Actions Workflows with cache and cache-dependency-path and different build commands for uploading to pypi.
  • Removing the now redundant requirements*.txt and MANIFEST.in.
  • Updating "read the docs" configuration.

@claudiofantacci claudiofantacci self-assigned this Apr 9, 2024
@claudiofantacci claudiofantacci added the enhancement New feature or request label Apr 9, 2024
@claudiofantacci
Copy link
Collaborator

Thanks for this PR! I'll try to have a look as soon as I can. In the meantime, can you have a look the CI that seems to be failing? Thanks again 🚀

@SauravMaheshkar
Copy link
Contributor Author

Thanks for this PR! I'll try to have a look as soon as I can. In the meantime, can you have a look the CI that seems to be failing? Thanks again 🚀

Addressed in 495b8a8, it was a typo 😅

@claudiofantacci
Copy link
Collaborator

Ok seems like that fixed it! 🚀
I'll have a look asap, but skimming through it LGTM 👍

@SauravMaheshkar
Copy link
Contributor Author

Also maybe it's worth mentioning but with optax copybara was throwing some errors and then ultimately someone had to add a empty pyproject.toml. If that is the case I can rebase the PR with a single squash commit to make everything nice and clean.

@claudiofantacci
Copy link
Collaborator

That's strange, in theory it should be a single squashed commit anyway.
In any case, given your prior with optax I would suggest doing that.
Can you please do it 😄 ?

@SauravMaheshkar
Copy link
Contributor Author

Happy to do it if necessary. Can you try running capybara on this PR?

Previously the GitHub CI had ran successfully however when attempting to merge, capybara was throwing an error. Can you try to run capybara on this PR? Incase there is an error I'll open another PR with an empty pyproject

Copy link
Collaborator

@claudiofantacci claudiofantacci left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, only a few minor things before approving 😄

pyproject.toml Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
test.sh Outdated Show resolved Hide resolved
@SauravMaheshkar
Copy link
Contributor Author

I addressed the comments in the recent commits.

@claudiofantacci claudiofantacci self-requested a review April 18, 2024 09:57
Copy link
Collaborator

@claudiofantacci claudiofantacci 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, let's see what Copybara tells us ahah 😄

@claudiofantacci
Copy link
Collaborator

I have a copybara failure, I'll look into it asap.

@SauravMaheshkar
Copy link
Contributor Author

I have a copybara failure, I'll look into it asap.

Hey @claudiofantacci, I'm assuming this is because of a lack of pyproject.toml in the master branch atm. Can you make a PR via copybara which adds a empty pyproject.toml ? Then I can rebase my PR and we can try again.

@claudiofantacci
Copy link
Collaborator

My Copybara is probably setup differently 😄
The error I get is because we're deleting some files (the requitements) that are expected.
I should be able to change this behaviour quite easily, but I'm juggling through a lot of things so this might require me a bit of time. I apologise! I'll get back here as soon as I can!

@claudiofantacci
Copy link
Collaborator

claudiofantacci commented Apr 22, 2024

Ok, I've prepared internal changes that we're testing to get this PR through, see #98 🚀

In the meantime, can you please update README.md according to the changes of this PR?

claudiofantacci

This comment was marked as duplicate.

Copy link
Collaborator

@claudiofantacci claudiofantacci left a comment

Choose a reason for hiding this comment

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

Ok the #98 is in, can you rebase and update the README.md?

@SauravMaheshkar
Copy link
Contributor Author

Sorry for the delay, I updated the README in 2a2d526

README.md Show resolved Hide resolved
Copy link
Collaborator

@claudiofantacci claudiofantacci left a comment

Choose a reason for hiding this comment

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

Amazing, thanks!

@copybara-service copybara-service bot merged commit 8c11743 into google-deepmind:master Apr 23, 2024
6 checks passed
@SauravMaheshkar SauravMaheshkar deleted the saurav/pyproject branch April 23, 2024 13:18
@SauravMaheshkar
Copy link
Contributor Author

Thanks for seeing this through @claudiofantacci. Cheers 😄 🥂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants