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

Prep support for running Pants with Python 3.10 or 3.11. #351

Merged
merged 16 commits into from
May 22, 2024

Conversation

kaos
Copy link
Member

@kaos kaos commented Jan 22, 2024

Look at the Python tag in the Pants PEX filename Cycle through all supported Python versions and check the PEX download URL using HEAD for each one to infer the Python version to use when installing Pants, when using a version of Pants that we don't already know which version of Python to use.

Drop support for PANTS_SHA. Done in #376.

Silence the install step, capturing the output to a pants-install.log file in the science cache. Done in #375.

Copy link
Member Author

@kaos kaos left a comment

Choose a reason for hiding this comment

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

@thejcannon if you're able, would love to get your eyes on my assumptions about our released pex files 😬 in case I'm wrong about anything here..

package/README.md Outdated Show resolved Hide resolved
package/src/main.rs Outdated Show resolved Hide resolved
tools/src/scie_pants/install_pants.py Show resolved Hide resolved
tools/src/scie_pants/pants_version.py Show resolved Hide resolved
tools/src/scie_pants/pants_version.py Outdated Show resolved Hide resolved
@kaos
Copy link
Member Author

kaos commented Jan 22, 2024

Likely want to add some nicer feedback in case you're on scie-pants 0.10.7 and try to use a newer pants that wants py 3.12.. I have no idea what that failure will look like now.

EDIT: It'll try to download a non-existing asset, and say as much, so the error message is giving enough details to show the issue, although it's not spelled out that upgrading scie-pants is the solution.

So.. good enough? :)

@kaos kaos marked this pull request as draft January 22, 2024 17:27
@kaos
Copy link
Member Author

kaos commented Jan 23, 2024

Uh... still need the find-links thing for Pants <2.. 😬

@kaos kaos marked this pull request as ready for review January 23, 2024 15:26
@kaos kaos requested a review from huonw January 23, 2024 15:27
@cburroughs
Copy link
Contributor

https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28

"The primary rate limit for unauthenticated requests is 60 requests per hour."

Just wanted to highlight the docs, I don't have a strong sense of how "risky" this is.

@thejcannon
Copy link
Member

Uh... still need the find-links thing for Pants <2.. 😬

Im happy to have a discussion about dropping v1 support. And telling people to use an older scie-pants for that.

@thejcannon
Copy link
Member

https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28

"The primary rate limit for unauthenticated requests is 60 requests per hour."

Just wanted to highlight the docs, I don't have a strong sense of how "risky" this is.

What's this in relation to? If it's the PEX downloading that isn't the API, and isnt limited.

@kaos
Copy link
Member Author

kaos commented Jan 24, 2024

https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28
"The primary rate limit for unauthenticated requests is 60 requests per hour."
Just wanted to highlight the docs, I don't have a strong sense of how "risky" this is.

What's this in relation to? If it's the PEX downloading that isn't the API, and isnt limited.

We use GH api requests to get the available assets for a release. But I think the number of requests will be relatively few, and cached, so ought to be OK in most cases.

@kaos
Copy link
Member Author

kaos commented Jan 24, 2024

Uh... still need the find-links thing for Pants <2.. 😬

Im happy to have a discussion about dropping v1 support. And telling people to use an older scie-pants for that.

I considered dropping v1 support as well. But then again, I don't feel it is motivated to drop it at this time, as it was working, and it didn't cost me much to preserve that functionality--I was just not aware of v1 being a thing still 😅

If it becomes difficult to maintain, I'm +1 to dropping it in a future version. Users still on Pants v1 already show they're OK staying on an old version, so feels like freezing their version of supported scie-pants should be no worse off.

Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Nice one.

I'd find it a bit easier to review if we split up the changes:

  • remove support for PANTS_SHA
  • fix the stdout bug

Is that too annoying to do? Thanks

tools/src/scie_pants/install_pants.py Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
tools/src/scie_pants/pants_version.py Show resolved Hide resolved
@kaos
Copy link
Member Author

kaos commented Feb 29, 2024

Nice one.

I'd find it a bit easier to review if we split up the changes:

  • remove support for PANTS_SHA
  • fix the stdout bug

Is that too annoying to do? Thanks

No, that's a good move. It kind of fell in my lap while going for the Py support changes.. but breaking those out makes sense.

kaos added a commit that referenced this pull request Mar 18, 2024
kaos added a commit that referenced this pull request Mar 18, 2024
The `PANTS_SHA` feature was deprecated with the release `0.10.0` of
`scie-pants`.

Broken out of #351
Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Exciting!

package/scie-pants.toml Outdated Show resolved Hide resolved
tools/src/scie_pants/pants_version.py Outdated Show resolved Hide resolved
tools/src/scie_pants/pants_version.py Outdated Show resolved Hide resolved
tools/src/scie_pants/pants_version.py Outdated Show resolved Hide resolved
tools/src/scie_pants/pants_version.py Outdated Show resolved Hide resolved
tools/src/scie_pants/install_pants.py Outdated Show resolved Hide resolved
tools/src/scie_pants/install_pants.py Outdated Show resolved Hide resolved
tools/src/scie_pants/pants_version.py Outdated Show resolved Hide resolved
tools/src/scie_pants/pants_version.py Outdated Show resolved Hide resolved
tools/src/scie_pants/install_pants.py Show resolved Hide resolved
tools/src/scie_pants/pants_python_versions.json Outdated Show resolved Hide resolved
tools/src/scie_pants/pants_version.py Outdated Show resolved Hide resolved
tools/src/scie_pants/pants_version.py Outdated Show resolved Hide resolved
tools/src/scie_pants/configure_pants.py Show resolved Hide resolved
@kaos kaos requested a review from huonw May 10, 2024 07:09
Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Really nice!

I like it a lot... however, it's a moderately complicated switch. We could consider doing a pre-release release containing this change (e.g. 0.12.0rc0) that we can manually test for a day or two before doing a proper full release.

In particular, I think get-pants.sh without --version will automatically pick up the "latest" release. Thus, if people do an unpinned install in CI (highly likely), every time we do a release we instantly start running the new code in people's CI. This means if we've made a mistake here, we'll have a bit of a panic to get it fixed up. By doing a non-"latest" pre-release first we'll have a bit of breathing room to validate.

Alternatively, we can just hope for the best! What do you think?

@kaos
Copy link
Member Author

kaos commented May 21, 2024

Alternatively, we can just hope for the best! What do you think?

I'm in favor of doing a pre-release. Prefer to avoid stressful bugs :p

@kaos
Copy link
Member Author

kaos commented May 21, 2024

@huonw I've added support for manually running the release workflow for a pre-release tag. The pre-release is not announced (besides to the GH discussions topic) nor published to our brew tap, so flies entirely under the radar. (had a bug so it wasn't marked as a pre-release properly, but I hope it was just my typo which is now fixed.)

Only way to use it is to explicitly download the binary from GH release assets and run/install it that way.

I've made a quick smoke test on a repo of mine using the 0.12.0-beta.0 version on Mac M2, seems to be working fine there at least.

❯ wget https://github.com/pantsbuild/scie-pants/releases/download/v0.12.0-beta.0/scie-pants-macos-aarch64

❯ ./scie-pants-macos-aarch64 -V
Bootstrapping Pants 2.19.1
Installing pantsbuild.pants==2.19.1 into a virtual environment at /Users/x/Library/Caches/nce/1077f2ed0a70d06f5815801851d6c094e975c4b07d42790e2055a5a4d2b52da1/bindings/venvs/2.19.1
New virtual environment successfully created at /Users/x/Library/Caches/nce/1077f2ed0a70d06f5815801851d6c094e975c4b07d42790e2055a5a4d2b52da1/bindings/venvs/2.19.1
12:39:27.49 [INFO] Initialization options changed: reinitializing scheduler...
12:39:30.04 [INFO] Scheduler initialized.
2.19.1

❯ PANTS_BOOTSTRAP_VERSION=report ./scie-pants-macos-aarch64
0.12.0-beta.0

And it picked the correct Python:

...a4d2b52da1/bindings/venvs/2.19.1/lib/python3.9/site-packages/pants/bin/native_client

@huonw
Copy link
Contributor

huonw commented May 22, 2024

Oh, cool!

My testing:

  • Works locally for me too:
    OS \ arch x86-64 arm64
    macOS ✅ (native) ✅ (native)
    Linux ✅ (docker, emulated) ✅ (docker)
  • I'm also running it through my work repo CI now (x86-64 Linux) seems to work fine too ✅

I feel like I have a pretty boring set-up though: everything connected to the internet always, no proxies/blocking. "Standard" operating systems etc. Do we want to solicit people to test more, especially if they have unusual set-ups?

Only way to use it is to explicitly download the binary from GH release assets and run/install it that way.

FYI looks like the get-pants.sh script we publish supports this with ./get-pants.sh --version 0.12.0-beta.0 too.

The pre-release is not announced

Minor process things, but I wonder if, for future, we could still announce these to Slack, similar to how we announce Pants dev releases. We can get this PR and its feature in, and iterate on the unrelated stuff once it's in, though.

@kaos
Copy link
Member Author

kaos commented May 22, 2024

Do we want to solicit people to test more, especially if they have unusual set-ups?

I'm willing to handle any edgy issues as they come up, as it seems to work fine for the general case it ought to work for most users. If there's too much issues, we can always roll back the latest version to 0.11.0 again to address them.

FYI looks like the get-pants.sh script we publish supports this with ./get-pants.sh --version 0.12.0-beta.0 too.

Awesome.

Minor process things, but I wonder if, for future, we could still announce these to Slack, similar to how we announce Pants dev releases. We can get this PR and its feature in, and iterate on the unrelated stuff once it's in, though.

Sounds good.

@kaos kaos merged commit 89981cd into main May 22, 2024
8 checks passed
@kaos kaos deleted the kaos/dyn-py-version branch May 22, 2024 07:36
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

4 participants