feat: migrate to pyproject.toml for building package

[SauravMaheshkar]

Request for Review: @hbq1 @rosshemsley

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 5 separate text files. It also allows for dynamic version allocation (reads automatically from /__init__.py).

I'd like to raise the discussion about shifting to a newer modern build backend in the deepmind JAX ecosystem (happy to raise subsequent PRs in other repositories as well)

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 updated cache-dependency-path (introduced in feat(ci/tests): bump setup-python version and enable cache #485) and different build commands for uploading to pypi.
• Removing the now redundant requirements/ directory and making subsequent updates to MANIFEST.in.
• Updating "read the docs" configuration.

[rosshemsley]

Thanks for writing this pull request! I'll try to get you a good review by end of the week :)

[rosshemsley]

Thanks for your pull request, it looks good to me.

Few things to think about:

Which tests have you run to check this works?

• Does installing directly from github work (still)?
• Can you successfully build a wheel and install the wheel etc.?
• Do the docs all still work and render the same way?
• Can this be submitted to pypi (this might not be something you can test - unless you use target a test-pypi)

I'd also like to get some of the other optax folks to take a look before approving :)

[SauravMaheshkar]

Thanks for your pull request, it looks good to me.

Few things to think about:

Which tests have you run to check this works?

• Does installing directly from github work (still)?
• Can you successfully build a wheel and install the wheel etc.?
• Do the docs all still work and render the same way?
• Can this be submitted to pypi (this might not be something you can test - unless you use target a test-pypi)

I'd also like to get some of the other optax folks to take a look before approving :)

• Installing from Github does work. I ran pip install git+https://github.com/SauravMaheshkar/optax.git, and it installed successfully.
• With the changes I've made in the Github Actions files, we can infact successfully build and install a wheel (python -m build)
• I've also made changes to the readthedocs.yml file which allows for installs using the new pyproject.toml file.

[rosshemsley]

Apologies for the slow responses @SauravMaheshkar! I have not forgotten about this, just trying to juggle a few projects at the moment :)

I see the CLA checker is complaining, do you know what the reason for this is? Have you followed the guidelines in https://github.com/deepmind/optax/blob/master/CONTRIBUTING.md?

[XuehaiPan]

I see the CLA checker is complaining, do you know what the reason for this is?

Re-trigger the workflow will resolve this.

[SauravMaheshkar]

Apologies for the slow responses @SauravMaheshkar! I have not forgotten about this, just trying to juggle a few projects at the moment :)

I see the CLA checker is complaining, do you know what the reason for this is? Have you followed the guidelines in https://github.com/deepmind/optax/blob/master/CONTRIBUTING.md?

I incorporated @XuehaiPan's suggestion into the PR and maybe that's causing the fail. But it's claimed in #513 (comment) that re-triggering the workflow should help fix it.

[XuehaiPan]

But it's claimed in #513 (comment) that re-triggering the workflow should help fix it.

@SauravMaheshkar Not sure why the CI complain about my CLA assignment. I have already assigned this. If it's not working, it's fine to squash the commit and remove my co-authorship of these commits (with a force-push).

[SauravMaheshkar]

And is the copybara import failing because of the squash 🙁

[SauravMaheshkar]

Similar change was just accepted in Flax google/flax#2960

[SauravMaheshkar]

Gentle ping

[rosshemsley]

Hello @SauravMaheshkar Apologies for being slow to respond.

We'd like to accept this, but the thing that's failing at the moment is that the system that syncs this with the internal version is failing.

This failure is happening because of the deleted files.

Do you think you could put those files back (requirements.txt, setup.py) etc, so that the import will succeed. We'll then be able to remove them in a way that will work on our end, and then finally get this patch in!

We did try to do this ourselves (hence commit from @mkunesch) but we think this would be easier if you could make the change and then we can work from there.

Does that work for you?

[SauravMaheshkar]

Hello @SauravMaheshkar Apologies for being slow to respond.

We'd like to accept this, but the thing that's failing at the moment is that the system that syncs this with the internal version is failing.

This failure is happening because of the deleted files.

Do you think you could put those files back (requirements.txt, setup.py) etc, so that the import will succeed. We'll then be able to remove them in a way that will work on our end, and then finally get this patch in!

We did try to do this ourselves (hence commit from @mkunesch) but we think this would be easier if you could make the change and then we can work from there.

Does that work for you?

I tried that in 93a7774, but it still seems to fail. But why though ??

[SauravMaheshkar]

@rosshemsley do you just want me to open up a new PR ??

[mkunesch]

It looks like we'll have to add an empty file structure first with an internal change and merge your PR on top - I'll submit a PR and let you know once it's done. Thanks a lot!

[mkunesch]

Okay, so this is done now in PR #526. I had to create the pyproject.toml file at the same time as making the internal change that tells copybara how to sync the file.

I've left the file empty in my PR so that your PR adds the content. I think the checks pass now so I think this worked!

Thanks a lot for your help with this!

[SauravMaheshkar]

Okay, so this is done now in PR #526. I had to create the pyproject.toml file at the same time as making the internal change that tells copybara how to sync the file.

I've left the file empty in my PR so that your PR adds the content. I think the checks pass now so I think this worked!

Thanks a lot for your help with this!

Thanks a lot for helping this through 😄

[SauravMaheshkar]

@rosshemsley any update on when we can get this in ?

[rosshemsley]

Hey @SauravMaheshkar!

Apologies again for taking a while to get around to this! I have been out of office on vacations, and so things got a little stalled.

The change is now shipped 🚢 🚢. Let's keep an eye on pypi and see if it works!

Thanks again for being patient with us on this!

[daskol]

@SauravMaheshkar FYI As far as I understand optax uses flit as a build backend so all metions of setuptools and requirements*.txt can be removed. Also I'm not sure about validity of readthedocs.yml.

[SauravMaheshkar]

@rosshemsley Now that this change has been in the repository for a while now. Do you think we can drop the redundant files which I initially removed in this PR ? Happy to open a PR for that change (unless we have a chance of encountering the problem again with copybara)

[rosshemsley]

Hey! @SauravMaheshkar,

I actually was looking at this yesterday - unluckily, there was a small problem with the deploy script with the new setup, which caused release 0.1.6 to fail. The fix was quite straight forwards (cfa6f78)

When I was looking at this, I was also looking at removing setup.py and the requirements, but didn't finish that yet (was hoping to find a way to test the publish flow before submitting, but haven't found a way yet).

Unfortunately, you're right that this would likely make our syncing tool unhappy (those files are mentioned directly).

[SauravMaheshkar]

Glad to know the fix was simple. No worries maybe you can remove it on your end (I encountered a similar problem while contributing to flax as well), as long as the changes are made and the project structure looks minimal it's good.

[SauravMaheshkar]

Hi @rosshemsley checking in again. I saw that recently optax bumped to a 0.1.8.dev on the master branch. Is it planned to perhaps shift to a entirely pyproject.toml based build (and thereby removing the other redundant files) in this upcoming version ?

[SauravMaheshkar]

Hi @rosshemsley checking in again. I saw that recently optax bumped to a 0.1.8.dev on the master branch. Is it planned to perhaps shift to a entirely pyproject.toml based build (and thereby removing the other redundant files) in this upcoming version ?

Gentle ping

[SauravMaheshkar]

@fabianp was wondering if you could provide any update on this ?

[fabianp]

thanks for brining that up @SauravMaheshkar. I would be fine in deleting the setup.py (and it seems that @rosshemsley and @mkunesch are in agreement with this), although it's currently still being used for some things like uploading to pypi in https://github.com/google-deepmind/optax/blob/main/.github/workflows/pypi-publish.yml

Would you be willing to submit a PR that removes setup.py and updates its use in https://github.com/google-deepmind/optax/blob/main/.github/workflows/pypi-publish.yml ?

Thanks!

[SauravMaheshkar]

thanks for brining that up @SauravMaheshkar. I would be fine in deleting the setup.py (and it seems that @rosshemsley and @mkunesch are in agreement with this), although it's currently still being used for some things like uploading to pypi in https://github.com/google-deepmind/optax/blob/main/.github/workflows/pypi-publish.yml

Would you be willing to submit a PR that removes setup.py and updates its use in https://github.com/google-deepmind/optax/blob/main/.github/workflows/pypi-publish.yml ?

Thanks!

Yes ofc more than happy to. It's just the "Check consistency between the package version and release tag" step that still depends. I'll make a PR for removing it.

[fabianp]

I very recently made a release. We should be in sync

…

On Sun, Jan 21, 2024, 17:03 Saurav Maheshkar ***@***.***> wrote: thanks for brining that up @SauravMaheshkar <https://github.com/SauravMaheshkar>. I would be fine in deleting the setup.py (and it seems that @rosshemsley <https://github.com/rosshemsley> and @mkunesch <https://github.com/mkunesch> are in agreement with this), although it's currently still being used for some things like uploading to pypi in https://github.com/google-deepmind/optax/blob/main/.github/workflows/pypi-publish.yml Would you be willing to submit a PR that removes setup.py and updates its use in https://github.com/google-deepmind/optax/blob/main/.github/workflows/pypi-publish.yml ? Thanks! Yes ofc more than happy to. It's just the "Check consistency between the package version and release tag" step that still depends. I'll make a PR for removing it. — Reply to this email directly, view it on GitHub <#513 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACDZB2OEW5FMNYEYU7XHWDYPU34BAVCNFSM6AAAAAAV3CMCDCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBSGY4DAMBZGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[SauravMaheshkar]

@fabianp It seems like we can't get the package version from the pyproject.toml file programmatically. So that check in the CI can't be done via a pyproject file.

IMO that setup shouldn't be necessary since flit picks up the package version.

[fabianp]

Thanks @SauravMaheshkar . How would you then modify the block

    - name: Check consistency between the package version and release tag
      run: |
        RELEASE_VER=${GITHUB_REF#refs/*/}
        PACKAGE_VER="v`python setup.py --version`"
        if [ $RELEASE_VER != $PACKAGE_VER ]
        then
          echo "package ver. ($PACKAGE_VER) != release ver. ($RELEASE_VER)"; exit 1
        fi

in the CI? BTW feel free to start a PR so we can test stuff

[SauravMaheshkar]

then modify the block

Not a modification, I think we'll have to drop that block. Am trying to figure out other workarounds, will open a PR if I find something

[SauravMaheshkar]

@fabianp now would you propose that I open a PR to delete the other files such as MANIFEST.in and setup.py, or would that cause some issue with internal google dev tooling ?

[fabianp]

Yes, please open the PR! I'll take care of things on the Google side