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

stop installing qiskit-terra from source in tox #1460

Merged
merged 3 commits into from
Feb 20, 2022

Conversation

hhorii
Copy link
Collaborator

@hhorii hhorii commented Feb 9, 2022

Summary

stop installing qiskit-terra from source in tox

Details and comments

This changes installation of qiskit-terra in tox.
Recent commit of #1448 changed installation of qiskit-terra to use pypi in most CI workflow, but not tox.

This changes installation of qiskit-terra in tox.
Recent commit of Qiskit#1448 changed installation of qiskit-terra to use pypi in most CI workflow, but not tox.
@hhorii hhorii requested a review from mtreinish February 9, 2022 15:31
@chriseclectic
Copy link
Member

I'm not sure if that change was intended @mtreinish can you comment? The original reason for installing qiskit from main was to catch when qiskit changes broke Aer before release, which used to happen quite frequently.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

You don't actually need to list qiskit-terra here, it's included in the requirements list for the project and will always be installed when pip installs aer. The additional line here was to pre-install terra from source outside of pip's normal behavior

tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@mtreinish
Copy link
Member

I'm not sure if that change was intended @mtreinish can you comment? The original reason for installing qiskit from main was to catch when qiskit changes broke Aer before release, which used to happen quite frequently.

It was very much intentional (see the commit message on: 1d19b67 ). I was always opposed to doing that because it puts the burden on the wrong project. Catching API breaks in terra should be terra's responsibility not aer's, especially because for that strategy to work we need someone to actively be on top of failures.

@garrison
Copy link
Member

it puts the burden on the wrong project. Catching API breaks in terra should be terra's responsibility not aer's

Why not add a workflow to Terra's CI that ensures that a given pull request does not break Aer's test suite? This would allow breakage to be detected and dealt with even sooner than is possible currently, rather than later (which is what #1448 does). Even if this Aer test within the Terra CI occurs on a single python version, it will detect the vast majority of changes that lead to Aer breaking.

@mtreinish
Copy link
Member

mtreinish commented Feb 14, 2022

The issue with that approach is that it doesn't scale. Terra ci is pretty oversubscribed on ci resources on a typical day just running the test matrix for its own testing. If we had to add a job for every downstream consumer's test suite we'd have very long wait times and never get anything merged. What you're describing though is the basic idea behind how qiskit-neko is going to be setup just for a subset of functionality we include in it (which should hopefully be more manageable).

@garrison
Copy link
Member

I didn't suggest adding every downstream consumer's test suite. I suggested adding Aer specifically to the Terra tests, and for one platform and version of Python only.

But yes, you see where I am going with this. I agree that testing every downstream package does not scale if we enforce that it must be done for every single pull request. But why not test the ones that look likely to be breaking? (And why should backwards compatibility promises be limited to the (small) subset of things qiskit-neko can test for when there is an entire corpus of open source Qiskit code available to test against?)

For this broader goal, I'm very much inspired by my experience developing Julia. In the Julia ecosystem, if a pull request to the core language is likely to be breaking, it is a regular practice to run CI for every registered Julia package against that proposed change before merging it, to detect breakage and, at times, performance regressions. It is also regular procedure to make sure that tests pass on all registered packages before making a release. Generally a patch-level release will not be made if it breaks any code at all (example), but there is a judgment call to be made when pull request is to be merged to master for the next minor release (example). Sometimes the breakage is determined to be worth it; other times it is not. Sometimes this knowledge leads to an even better PR, and other times it leads to pull requests to other projects. Regardless of result, doing this level of testing provides a crucial window into what will break throughout the ecosystem from any given PR, before it is merged. The Julia community instituted this type of CI system when it was a very young ecosystem, and I've watched as it has paid immense dividends over time.

If Qiskit were to achieve something similar, I still expect qiskit-neko would be still very valuable for testing on multiple backend providers.

@mtreinish
Copy link
Member

I think we can have a separate issue (probably on terra) to discuss this as this is off-topic for this mostly mechanical fix. I agree having some automated opt-in testing of the wider downstream ecosystem is very valuable. I wasn't familiar with how julia's testing was setup, but I've seen similar systems before. Rust has a similar opt-in mechanism they use around release time to test compilation of everything on crates.io. The issue with this for qiskit right now is we don't have the automation systems in place to do that kind of testing and it would have to be built (there are also mechanical issues around how we find the wider downstream, install them, and test them which isn't as simple as in a standalone language like julia or rust that have their own package repos). The idea behind having more coupled integration tests running pre-merge on every PR is more an acknowledgement that the qiskit-* projects under the qiskit github org are more tightly coupled and we want to ensure that as a cohesive unit that is always working. To that end having an integration test suite run on every commit is what I was planning for as a first step because that will get us a lot and also should make dealing with this api breakages between qiskit projects easier to manage.

@mtreinish mtreinish merged commit 9f78243 into Qiskit:main Feb 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants