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

Move build configuration to static configuration files #10958

Merged
merged 5 commits into from
Nov 29, 2023

Conversation

jakelishman
Copy link
Member

Summary

This removes the vast majority of the dynamic code from our setup.py files, and instead uses static configuration to specify them. As part of this, we also use a single VERSION.txt as a single source of truth for both the qiskit-terra main package and the qiskit "metapackage". This should avoid a situation where we let the two get out of sync during a release, which can cause us severe problems for fixing the problem with respect to git tags.

Summary

Details and comments

@jakelishman jakelishman added type: qa Issues and PRs that relate to testing and code quality Changelog: None Do not include in changelog labels Oct 3, 2023
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Nov 28, 2023

Pull Request Test Coverage Report for Build 7037526759

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 5 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.003%) to 87.441%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 93.76%
crates/qasm2/src/lex.rs 4 91.67%
Totals Coverage Status
Change from base Build 7035250835: -0.003%
Covered Lines: 59766
Relevant Lines: 68350

💛 - Coveralls

This removes the vast majority of the dynamic code from our `setup.py`
files, and instead uses static configuration to specify them.  As part
of this, we also use a single `VERSION.txt` as a single source of truth
for both the `qiskit-terra` main package and the `qiskit` "metapackage".
This should avoid a situation where we let the two get out of sync
during a release, which can cause us severe problems for fixing the
problem with respect to git tags.
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.

Overall this LGTM, it's am much simpler state for managing the package. Using static config files for package metadata makes a lot more sense. Just a few small inline comments. Also do you know if this changes the minimum pip version we can use?

Comment on lines +15 to +17
"quantum circuit",
"quantum computing",
"quantum programming language",
Copy link
Member

Choose a reason for hiding this comment

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

These are good additions. While we're modifying the metadata, did you want anything compiler related here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I think I just made sure it was synchronised with the tags on the GitHub repo, rather than going out my way to add things. We can add "quantum compiler" to both if you thing it'd be good?

Copy link
Member

Choose a reason for hiding this comment

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

Nah it's fine, we can always tweak these later if needed.

pyproject.toml Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
# This will make the resolution work for installers from PyPI, but `pip install .[all]` will be
# unreliable because `qiskit-terra` will resolve to the PyPI version, so local changes in the
# optionals won't be reflected.
all = ["qiskit[qasm3-import,visualization,crosstalk-pass,csp-layout-pass]"]
Copy link
Member

Choose a reason for hiding this comment

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

This seems less than ideal, but in practice I guess people aren't using the all extra very frequently. I can't think of a better way to do this besides copy and pasting the component lists for all too. I'm guessing we can't use dynamic for optional dependencies because that's my only other thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's not super ideal, though I was assuming .[all] isn't being used at all.

pyproject.toml Outdated Show resolved Hide resolved
dependencies = {file = "requirements.txt" }

[tool.setuptools.packages.find]
include = ["qiskit", "qiskit.*"]
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't exclude = [""test*"] be more consistent with what we were doing before?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be more consistent and I can switch it over to that if you prefer. I think the include-list form is maybe a bit less liable to silently break than an exclude list, though.

Copy link
Member

Choose a reason for hiding this comment

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

That's fine with me, this works.

@mtreinish mtreinish added this to the 1.0.0 milestone Nov 28, 2023
mtreinish
mtreinish previously approved these changes Nov 29, 2023
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.

This LGTM now just one quick question inline, but I don't think it makes a big difference either way.

name = "qiskit"
description = "An open-source SDK for working with quantum computers at the level of extended quantum circuits, operators, and primitives."
requires-python = ">=3.8"
license = { file = "LICENSE.txt" }
Copy link
Member

Choose a reason for hiding this comment

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

Should we add?

Suggested change
license = { file = "LICENSE.txt" }
license = { file = "LICENSE.txt", text = "Apache 2.0" }

I'm not sure if this is valid syntax or not though. I was doing one final pass through comparing the old setup.py with the metadata in the pyproject.toml and noticed we had the license string set before (and the file was pulled in implicitly I think) but now we just point to the file. But I also need to read PEP 639 to see if this matters at all either.

Copy link
Member Author

Choose a reason for hiding this comment

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

The current PyPA guidelines say it's either-or (though maybe others interpret that text differently), so maybe it's best to just revisit when PEP 639 is accepted?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we can look at it again if/when pep 639 is accepted.

@jakelishman
Copy link
Member Author

For the top question, which I didn't answer: I don't think this changes anything about the pip version, though it might require some newer setuptools features to support the dynamic tables. I don't know what the minimum version of it is, though - setuptools' documentation isn't great at that.

@mtreinish mtreinish added this pull request to the merge queue Nov 29, 2023
Merged via the queue into Qiskit:main with commit 4c9cdee Nov 29, 2023
13 checks passed
@jakelishman jakelishman deleted the static-setup branch November 29, 2023 23: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