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

Optional module dependencies are requirements-dev? #3654

Closed
1ucian0 opened this issue Dec 25, 2019 · 3 comments · Fixed by #8967
Closed

Optional module dependencies are requirements-dev? #3654

1ucian0 opened this issue Dec 25, 2019 · 3 comments · Fixed by #8967

Comments

@1ucian0
Copy link
Member

1ucian0 commented Dec 25, 2019

It seems that we don't have a clear policy on how to handle requirements for "optional" modules.

For example #3532 introduced a pass that requieres z3-solver an that was added as a requirements-dev https://github.com/Qiskit/qiskit-terra/pull/3532/files#diff-f4c265073ea4274a80eca85440875e56, while #3043 also introduced a pass with an extra dependency (python-constraint in this case) but did not extend requirements-dev.

What should be the policy here?

@mtreinish
Copy link
Member

One thing I think we need to ensure is that we have these optional requirements installed in CI for testing. An issue that is very easy to envision is that we end up skipping all the tests for optional requirements because they're not present. Then we never test things anywhere. I felt that putting the optional requirements in requirements-dev.txt and having the tests unconditionally run felt like the right approach. That way we're always running all the tests in CI. While some features (like the passes mentioned here) look self contained they do rely on base constructs like the dag, passmanager, etc so it's good to make sure that we don't accidentally break these passes.

@1ucian0
Copy link
Member Author

1ucian0 commented Jan 2, 2020

Good point. Would having a requirements-optional.txt (with comments about which module needs each dependency) too overkilling?

@jakelishman
Copy link
Member

Ha, I was just looking through old issues and found this. Turns out that great minds think alike. Will be fixed by #8967.

@jakelishman jakelishman linked a pull request Apr 19, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants