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

Remove unnecessary pip install in CI #9756

Closed

Conversation

Eric-Arellano
Copy link
Collaborator

Summary

For the lint job, we already are installing Terra via an editable install:

https://github.com/Qiskit/qiskit-terra/blob/fa7ac688fdce3ecc27fb7e13d8b2c3f8553b3ff8/.azure/lint-linux.yml#L25-L27

No need to reinstall Terra again. The Aer installation respects the prior editable install:

Requirement already satisfied: qiskit-terra>=0.21.0 in ./test-job/lib/python3.11/site-packages (from qiskit-aer) (0.24.0)

Details and comments

#8501 removed several calls to python setup.py build_ext --inplace because they already were covered by the editable install on a prior line. But for the lint job, it replaced that line with pip install -e . -- I suspect it was only a mistake not realizing we already had the editable install a few lines above.

@Eric-Arellano Eric-Arellano added type: qa Issues and PRs that relate to testing and code quality Changelog: None Do not include in changelog labels Mar 8, 2023
@Eric-Arellano Eric-Arellano requested a review from mtreinish March 8, 2023 18:17
@Eric-Arellano Eric-Arellano requested a review from a team as a code owner March 8, 2023 18:17
@qiskit-bot
Copy link
Collaborator

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we're properly considering the safety, we actually want all the pip install lines collapsing onto a single line. We ought to force pip's resolver to consider all the constraints simultaneously, since it can get itself into an inconsistent state including changing prior fixed versions if it's called multiple times.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4367280039

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.08%) to 85.284%

Totals Coverage Status
Change from base Build 4367188637: -0.08%
Covered Lines: 67577
Relevant Lines: 79238

💛 - Coveralls

@Eric-Arellano Eric-Arellano deleted the ci-install-terra-from-source branch March 8, 2023 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog type: qa Issues and PRs that relate to testing and code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants