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

feat(ci): drop setup.py from publishing CI #737

Merged
merged 2 commits into from
Jan 26, 2024
Merged

feat(ci): drop setup.py from publishing CI #737

merged 2 commits into from
Jan 26, 2024

Conversation

SauravMaheshkar
Copy link
Contributor

Context

This PR aims to continue the work done in #513 to remove setup.py and migrate to a pure pyproject.toml based build. While the PR was merged in (#513 (comment)) the older legacy files such as setup.py and MANIFEST.in still exist.

The comment to delete such legacy files was raised again #513 (comment) and thus the issue to drop setup.py from CI raised in #513 (comment).


Changes made my this PR can be summarised as follows:

  • switch from dynamic to statically defined version metadata in pyproject.toml.
  • Modify publishing workflow to read version metadata from pyproject.toml.

Request for Review: @fabianp

@fabianp
Copy link
Member

fabianp commented Jan 25, 2024

thanks @SauravMaheshkar . This has the big disadvantage that the versions are now duplicated in both optax/init.py and in the pyproject.toml . How about getting the version with python -c "import optax; print(optax.__version__)"? Would something like that work?

@SauravMaheshkar
Copy link
Contributor Author

thanks @SauravMaheshkar . This has the big disadvantage that the versions are now duplicated in both optax/init.py and in the pyproject.toml . How about getting the version with python -c "import optax; print(optax.__version__)"? Would something like that work?

Yes that would work, however that would require for us to do a editable install in that step i.e. a pip install -e .

Although with some cache I'm sure even that could be sped up. Would you recommend I proceed with the proposed fix ?

@fabianp
Copy link
Member

fabianp commented Jan 25, 2024

do we really need to install optax for that? I think as long as we run it from the optax root directory it should work

@SauravMaheshkar
Copy link
Contributor Author

We would run into import error's right ? Following is my terminal output for running python -c "import optax; print(optax.__version__)"

❯ python -c "import optax; print(optax.__version__)"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/saurav/github/SauravMaheshkar/optax/optax/__init__.py", line 17, in <module>
    from optax import contrib
  File "/Users/saurav/github/SauravMaheshkar/optax/optax/contrib/__init__.py", line 17, in <module>
    from optax.contrib.cocob import cocob
  File "/Users/saurav/github/SauravMaheshkar/optax/optax/contrib/cocob.py", line 23, in <module>
    import jax.numpy as jnp
ModuleNotFoundError: No module named 'jax'

@fabianp
Copy link
Member

fabianp commented Jan 25, 2024

works for me, it seems you're just missing some dependencies

@SauravMaheshkar
Copy link
Contributor Author

SauravMaheshkar commented Jan 25, 2024

The python environment in the Github Actions workflow won't have jax installed, right ? It only installs setuptools, wheel, twine and build in the previous step.

Sorry if I'm missing something 😅

@fabianp
Copy link
Member

fabianp commented Jan 25, 2024

yes, you're absolutely right. I was thinking we could run pip install . right before python -c "import optax; print(optax.__version__)"

@fabianp
Copy link
Member

fabianp commented Jan 25, 2024

which I see is what you proposed a few comments earlier 🙃 . Anyway, we seem to be on the same page 😄

@SauravMaheshkar
Copy link
Contributor Author

Sounds good, will revert the change to pyproject.toml and do a editable install before fetching the version.

@fabianp
Copy link
Member

fabianp commented Jan 25, 2024

My point before is that we don't really need to install optax, just the dependencies. Not sure if there's an easy way to do that, and TBH doesn't change much

@fabianp
Copy link
Member

fabianp commented Jan 25, 2024

I thought pip install . does that, install the dependencies and not the package, but I could be wrong

@fabianp
Copy link
Member

fabianp commented Jan 26, 2024

Sounds good, will revert the change to pyproject.toml and do a editable install before fetching the version.

Thanks! Let me know when I should take another look at it

@SauravMaheshkar
Copy link
Contributor Author

CI looks green. Should be good now @fabianp

@fabianp
Copy link
Member

fabianp commented Jan 26, 2024

Thanks! Looks good to me

@copybara-service copybara-service bot merged commit c4a1602 into google-deepmind:main Jan 26, 2024
6 checks passed
@SauravMaheshkar SauravMaheshkar deleted the ci-drop-setuppy branch January 26, 2024 17:58
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.

2 participants