-
-
Notifications
You must be signed in to change notification settings - Fork 963
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
Migration to pyproject.toml
for packaging
#4164
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4164 +/- ##
=======================================
Coverage 90.43% 90.43%
=======================================
Files 172 172
Lines 13660 13660
=======================================
Hits 12354 12354
Misses 1306 1306 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
I got the feedback (from core-dev team) that we can go ahead for this transition, so let me remove |
pyproject.toml
for packagingpyproject.toml
for packaging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand it's WIP but left a single request change since I noticed while reviewing another PRs.
It's required to fix all actions that uses actions/cache
to cache ~/.cache/pip
since hashFiles('**/setup.py')
is included in the cache key.
https://github.com/optuna/optuna/blob/master/.github/workflows/tests-storage.yml#L69-L77
I updated pyproject.toml based on the changed in setup.py.
Thank you, I just updated to use |
@HideakiImamura Could you review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments while taking a look quickly. Also Could you remove setup.py
?
"Programming Language :: Python :: 3.7", | ||
"Programming Language :: Python :: 3.8", | ||
"Programming Language :: Python :: 3.9", | ||
"Programming Language :: Python :: 3.10", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In setup.py
, "Programming Language :: Python :: 3.11" is also listed.
Co-authored-by: Masashi Shibata <c-bata@users.noreply.github.com>
Oh, sorry. It restored in syncing the master branch... Removed. |
@HideakiImamura @c-bata I merged the master branch and tests passed. |
@HideakiImamura @gen740 |
[tool.setuptools.packages.find] | ||
include = ["optuna*"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Describing package discovery in pyproject.toml
is in the beta stage.
https://setuptools.pypa.io/en/latest/userguide/package_discovery.html
If we'd like to avoid the unstable API for building a package, setup.cfg
maybe a better place for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to use this simple rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update and investigation! Almost, LGTM!
We have planned to release optuna v3.1.0 in next Monday. Just to be safe, I think this PR, which has a large impact around installation, should be merged after the 3.1.0 release.
pyproject.toml
Outdated
"mlflow", | ||
"pandas", | ||
"pillow", | ||
"plotly>=4.0.0", # optuna/visualization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version is constrained as plotly>=4.9.0
in the master branch. https://github.com/optuna/optuna/blob/master/setup.py#L74
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, updated in 2379003. 🙇
pyproject.toml
Outdated
optional = [ | ||
"matplotlib!=3.6.0", # optuna/visualization/matplotlib | ||
"pandas", # optuna/study.py | ||
"plotly>=4.0.0", # optuna/visualization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
And, CI errors will be fixed in #4301. |
Thank you for the careful consideration, I agree with you that merge the PR after the next release for safe. 👍 @HideakiImamura @gen740 |
pyproject.toml based project (PEP 621) has supported setuptools >= 61.0.0. So it must not be able to even build the project on setuptools=56. I suspect you are looking different setuptools version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update. LGTM. Note that the CI will be fixed by merging the master.
Could you merge the master branch? |
Note that setup.py has been updated, so you need to update pyproject.toml as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Hmm..., it seemingly succeeded to install in editable manner with https://gist.github.com/himkt/39a48eff132107b56e125312eb117485 Building the package is done in the isolated environment, which is why we can build the package with old pip/setuptools. https://gist.github.com/himkt/f441968e9ca9406dabb7c25a5d1a7fe8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I found a missed todo comment, but it can be added as a follow-up.
"plotly>=4.9.0", # optuna/visualization. | ||
"scikit-learn", | ||
"scikit-optimize", | ||
"sphinx<6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed the todo comment above this line.
# TODO(not522): Remove the constraint after sphinx_rtd_theme supports Sphinx 6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙇 fixed in f81457c.
@himkt I'm so sorry for the delayed response, but could you merge the master to run the Checks CI? |
Motivation
This PR migrates from
setup.py
topyproject.toml
for the packaging configuration file. With this migration, we can easily retrieve the package metadata using TOML parser (it's tricky withsetup.py
since we need to parse a Python script). The ecosystem of packaging Python project withpyproject.toml
is getting practical so I propose to start defining the package metadata withpyproject.toml
.Description of the changes
References
reuires-python
: https://stackoverflow.com/questions/72373093package-data
: https://stackoverflow.com/questions/69647590/