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

Factor out checks-superstaq #789

Merged
merged 42 commits into from
Oct 2, 2023
Merged

Factor out checks-superstaq #789

merged 42 commits into from
Oct 2, 2023

Conversation

perlinm
Copy link
Collaborator

@perlinm perlinm commented Sep 23, 2023

image

@perlinm
Copy link
Collaborator Author

perlinm commented Sep 23, 2023

There's definitely a lot going on in .github/workflows/ci.yaml that I don't understand. Help appreciated 😅.
I think my biggest confusion is when we should pip install -e ...-superstaq[dev] vs. without [dev].

Also: how should we control versioning of checks-superstaq? Related. EDIT: solved with
image

@@ -50,7 +50,7 @@ jobs:
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install -e ./general-superstaq[dev] -e ./qiskit-superstaq -e ./cirq-superstaq -e ./supermarq-benchmarks
pip install -e ./checks-superstaq -e ./general-superstaq -e ./qiskit-superstaq -e ./cirq-superstaq -e ./supermarq-benchmarks
Copy link
Collaborator Author

@perlinm perlinm Sep 23, 2023

Choose a reason for hiding this comment

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

Is there a particular reason for the pylint to not install [dev] versions?

Copy link
Contributor

Choose a reason for hiding this comment

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

nope, mostly there was just no reason to

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess no need to add it then? Should I remove [dev] from some other places while we're at it? Maybe it doesn't matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

it shouldn't really matter much i think - in practice the dev versions are often installed anyway because we usually use a shared cache. leaving it off can make that check a little faster after a cache miss, but the difference is probably pretty small

@perlinm
Copy link
Collaborator Author

perlinm commented Sep 23, 2023

UPDATE: Fixed with the third option below.

OLD:

The requirements check is failing because checks-superstaq is not on PyPI. So it's a chicken-and-egg problem. Maybe we comment out the requirements check in this PR, version bump in a second PR (which should deploy to PyPI), and then un-comment this check in a third PR?

Actually, I guess we can version bump in this PR(?)

Third option: if a package is installed locally and not found on PyPI, just check against the local version. I think I'll do this actually.

Comment on lines +203 to +207
try:
package_data = urllib.request.urlopen(pypi_url).read().decode()
pypi_version = json.loads(package_data)["info"]["version"]
except urllib.error.URLError:
return checks_superstaq.__version__
Copy link
Collaborator Author

@perlinm perlinm Sep 24, 2023

Choose a reason for hiding this comment

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

This is really a temporary fix for the fact that the requirements.py check would otherwise fail until checks-superstaq is on PyPI. Not unreasonable since we take the max of the PyPI and local package versions below anyways.

A soon-to-follow PR will change some things about requirements.py anyways, to make it independent of the other *-superstaq packages.

@perlinm
Copy link
Collaborator Author

perlinm commented Sep 24, 2023

I presume we should version bump to 0.5.0 in this PR?

Copy link
Contributor

@richrines1 richrines1 left a comment

Choose a reason for hiding this comment

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

general-superstaq[dev] should be replaced with checks-superstaq in */dev-requirements.txt. Otherwise, just a few minor questions!

checks-superstaq/pyproject.toml Outdated Show resolved Hide resolved
check/all_.py Outdated
import sys

import general_superstaq.check
import checks_superstaq as check
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity - any reason for check instead of checks?

Copy link
Collaborator Author

@perlinm perlinm Oct 2, 2023

Choose a reason for hiding this comment

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

Nope. Maybe "this repo checks superstaq" is a fun play on words 😄?

I am happy to make this check-superstaq though, and even to consider different names entirely -- the naming collision between ./checks-superstaq/ and ./check/all_.py (for example) is a 👎 for tab completion.

Copy link
Contributor

Choose a reason for hiding this comment

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

haha i like "checks-superstaq" - i was mostly wondering if it would be more consistent to import it with as checks instead of as check (doesn't help with the tab-completion point though)

Copy link
Collaborator Author

@perlinm perlinm Oct 2, 2023

Choose a reason for hiding this comment

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

Ah, I see. Yeah import ... as checks makes sense to me

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I see I read your original comment backwards lol (swapping check and checks)

@@ -5,7 +5,7 @@ authors = [
{ name = "Superstaq development team", email = "superstaq@infleqtion.com" },
]
readme = { file = "README.md", content-type = "text/markdown" }
license = { text = "Apache 2" }
license = { text = "Apache-2.0" }
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change? fwiw cirq uses "Apache 2" and qiskit uses "Apache 2.0" (with no hyphen)

Copy link
Collaborator Author

@perlinm perlinm Oct 2, 2023

Choose a reason for hiding this comment

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

I see Apache-2.0 on github, for example.

This is definitely not a big deal either way though.

Copy link
Collaborator Author

@perlinm perlinm Oct 2, 2023

Choose a reason for hiding this comment

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

I guess for the sake of neater git blame type stuff I'll leave it as it was before haha nvm

Copy link
Contributor

Choose a reason for hiding this comment

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

ah good find, Apache-2.0 seem right then

Copy link
Contributor

@richrines1 richrines1 left a comment

Choose a reason for hiding this comment

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

nice - this looks solid afaict

i think we should manually deploy checks-superstaq 0.4.25 to pypi, so from now it will be updated normally along with version bumps

@perlinm
Copy link
Collaborator Author

perlinm commented Oct 2, 2023

Why not just version bump in this PR? Wouldn't that deploy? I feel like it doesn't make sense to deploy checks-superstaq without simultaneously version bumping gss.

Nevermind. No reason to version bump in this PR.

@perlinm perlinm merged commit 0a9f1eb into main Oct 2, 2023
16 checks passed
@perlinm perlinm deleted the factor-out-checks branch October 2, 2023 20:06
bharat-thotakura pushed a commit that referenced this pull request Oct 5, 2023
![image](https://github.com/Infleqtion/client-superstaq/assets/567092/523e915e-351a-4265-aa2a-f095ad7eb084)

---------

Co-authored-by: richrines1 <85512171+richrines1@users.noreply.github.com>
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.

2 participants