-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add extras mechanism for finer-grained dependency selection #45
base: main
Are you sure you want to change the base?
Conversation
fb214fc
to
3fc43e5
Compare
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #45 +/- ##
==========================================
- Coverage 91.36% 91.35% -0.01%
==========================================
Files 42 43 +1
Lines 1737 1782 +45
==========================================
+ Hits 1587 1628 +41
- Misses 150 154 +4
|
e88ec9c
to
57f8e1e
Compare
57f8e1e
to
83f8dc0
Compare
60f33d9
to
c05b578
Compare
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.
Concept ACK, however I was not able to install the project at this PR's revision.
Python 3.10, poetry v1.5.1:
$ poetry env use python3.10
$ poetry shell
$ poetry install
Installing dependencies from lock file
Package operations: 2 installs, 0 updates, 0 removals
• Installing codecov (2.1.12): Failed
RuntimeError
...
• Installing gpy (1.10.0 f63ed48): Failed
AssertionError
...
For python 3.11, instead pyproject.toml
needs to be updated to menion compatibility with python == 3.11.
I see you did this in feat/python3.11-bis
. However, in that branch poetry.lock
needs to be regenerated. Trying to regenerate it via `poetry install fails with:
Resolving dependencies... (94.7s)
Because black-it depends on GPy (^1.10.0) which doesn't match any versions, version solving failed.
I'd say we can start rebasing the PR on top of the current tip and move from there.
pyproject.toml
Outdated
GPy = { version = "^1.10.0", optional = true } | ||
xgboost = { version = "^1.7.2", optional = true } |
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 did not remember if this PR was written before poetry had support for optional groups.
I then checked, and the "extras" syntax used here appears - indeed - to be the right one (from https://python-poetry.org/docs/master/managing-dependencies/):
Dependency groups other than the implicit main group, must only contain dependencies you need in your development process. Installing them is only possible by using Poetry.
To declare a set of dependencies, which add additional functionality to the project during runtime, use extras instead. Extras can be installed by the end user using pip.
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.
yes, we want to rely on the setuptools' extras mechanism, which is poetry-agnostic and just works with pip
.
This paradigm has been used by other successful projects, e.g. OpenAI Gym: https://github.com/openai/gym#installation
bd5c2fe
to
72829de
Compare
I added a couple of tests (24bc95e and 72829de) to verify that the dependencies checks actually work. The commit 7e51add just skips a test on Windows due to flaky behaviour (see https://github.com/bancaditalia/black-it/actions/runs/5952400728/job/16144300857?pr=45#step:6:2581). |
Given that the change has also been documented in the README (see c740c3c#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R54-R68), I think the PR is in a good state to be reviewed and then eventually merged. |
c716659
to
2b2f64a
Compare
REBASE: GPy was temporarily left as-is, it should be removed. Inspired by: #36 (comment). This commit includes the extra-dependencies mechanism of setuptools to overcome limitations specific to certain dependencies (e.g. no support for some Python interpreter versions). The changes use the following conventions for extras names: - `[all]`: install all dependencies from all extras - `[X-sampler]`: install all dependencies to make X sampler to work - `[X-loss]`: install all dependencies to make X loss function to work. We do not have yet an example for the last item for the moment; but for "forward-compatibility" of the nomenclature, we leave the -sampler suffix. E.g. for GPy, we could have the extra called gp-sampler, that installs GPy on-demand, and not installed if not needed by the user. This commit also includes a mechanism to handle import errors for the non-installed dependencies for some component. Such mechanism provides a useful message to the user, e.g. it raises an exception with a useful error message pointing out to the missing extra in its local installation of black-it.
2b2f64a
to
c4fa003
Compare
Proposed changes
Inspired by: #36 (comment).
This commit includes the extra-dependencies mechanism of setuptools to overcome limitations specific to certain dependencies (e.g. no support for some Python interpreter versions).
The changes use the following conventions for extras names:
[all]
: install all dependencies from all extras[X-sampler]
: install all dependencies to make X sampler to work[X-loss]
: install all dependencies to make X loss function work.We do not have yet an example for the last item for the moment; but for the "forward-compatibility" of the nomenclature, we leave the
-sampler
suffix.E.g. for GPy, we could have the extra called
gp-sampler
, that installs GPy on-demand, and not installed if not needed by the user.This commit also includes a mechanism to handle import errors for the non-installed dependencies for some components. Such a mechanism provides a useful message to the user, e.g. it raises an exception with a useful error message pointing out the missing extra in its local installation of black-it.
Fixes
Partially addresses #36
Types of changes
What types of changes does your code introduce?
Put an
x
in the boxes that applyChecklist
n/a
Further comments
This PR is mainly open for discussion, rather than a definitive contribution.