-
Notifications
You must be signed in to change notification settings - Fork 204
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
feat(ci): drop setup.py
from publishing CI
#737
Conversation
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 |
Yes that would work, however that would require for us to do a editable install in that step i.e. a Although with some cache I'm sure even that could be sped up. Would you recommend I proceed with the proposed fix ? |
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 |
We would run into import error's right ? Following is my terminal output for running ❯ 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' |
works for me, it seems you're just missing some dependencies |
The python environment in the Github Actions workflow won't have Sorry if I'm missing something 😅 |
yes, you're absolutely right. I was thinking we could run |
which I see is what you proposed a few comments earlier 🙃 . Anyway, we seem to be on the same page 😄 |
Sounds good, will revert the change to |
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 |
I thought |
Thanks! Let me know when I should take another look at it |
CI looks green. Should be good now @fabianp |
Thanks! Looks good to me |
Context
This PR aims to continue the work done in #513 to remove
setup.py
and migrate to a purepyproject.toml
based build. While the PR was merged in (#513 (comment)) the older legacy files such assetup.py
andMANIFEST.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:
pyproject.toml
.pyproject.toml
.Request for Review: @fabianp