-
Notifications
You must be signed in to change notification settings - Fork 116
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
Move requirements into subpackages #230
Conversation
Change-Id: I48b65062fbc2f1aa59749977bce54b5eec23a819
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.
Seems fine but for two comments.
setup.py
Outdated
for r in extras_require | ||
} | ||
|
||
# TODO: remove and require users to install via extras_require. |
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.
Should this link to a Github issue?
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.
done, also see #231
|
||
install_requires = _parse_requirements(pathlib.Path('requirements.txt')) | ||
extras_require = [ | ||
'otoc', 'qaoa', 'optimize', 'hfvqe', 'fermi_hubbard' |
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.
Is there a good way to auto-detect this (or alternatively, for keeping this up to date if we add more experiments)?
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, I first wrote it to glob/regex for extra-requirements.txt and I could set it to that. However: for my next thing I want to add CI jobs that tests each subpackage in isolation. Autogenerating CI jobs would likely be more complicated than its worth. Especially since there will be an isolated "vanilla deps" build that will run all tests not in the named extra-requires subpackages which would fail if newly added experiments did indeed have extra requirements.
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.
Ok, SGTM, I already approved this PR. Feel free to merge when ready.
Change-Id: I1921c0a2e866679a96549aee2d868a1478a24020
extra-requirements.txt
per subpackageinstall_requires
and addextra_requires
for each subpackage (setup.py)recirq[qaoa]
. Remove extra requires frominstall_requires
. This stepwise change is necessary since the notebooks install from github Separate extra requirements for specific experiments #231Annotated change: