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

Add a JSON output for pyodide xbuildenv search, better tabular output #28

Merged

Conversation

agriyakhetarpal
Copy link
Member

Description

This PR closes #26. It adds a pyodide xbuildenv search --json option to print a JSON-based output, along with associated tests. The advantage is that it can be saved to a file, piped to jq or shell functions (or any equivalent tools), or simply imported into a Pythonic interface.

Additionally, I added a small context manager that does not do anything but stop printing the following lines:

Starting new HTTPS connection (1): raw.githubusercontent.com:443
https://raw.githubusercontent.com:443 "GET /pyodide/pyodide/main/pyodide-cross-build-environments.json HTTP/11" 200 917

in the output, because it conflicts with receiving valid JSON. Please let me know if this would be undesirable. If yes, I'll try to work around it so that it gets printed for the non-JSON output (table).

@agriyakhetarpal
Copy link
Member Author

Also, I got slightly nerdswiped by the tabular outputs we have, and implemented a prettified version of it (from https://ozh.github.io/ascii-tables/) in ce1904a, see below:

pyodide xbuildenv search on the main branch

Version         Python          Emscripten      pyodide-build                   Compatible
----------      ----------      ----------      -------------------------       ----------
0.27.0a2        3.12.1          3.1.58          0.26.0 -                        Yes       
0.26.2          3.12.1          3.1.58          0.26.0 -                        Yes       
0.26.1          3.12.1          3.1.58          0.26.0 -                        Yes       
0.26.0          3.12.1          3.1.58          0.26.0 -                        Yes       

pyodide xbuildenv search with Unicode symbols

┌────────────┬────────────┬────────────┬───────────────────────────┬────────────┐
│ Version    │ Python     │ Emscripten │ pyodide-build             │ Compatible │
├────────────┼────────────┼────────────┼───────────────────────────┼────────────┤
│ 0.27.0a2   │ 3.12.1     │ 3.1.58     │ 0.26.0 -                  │ Yes        │
│ 0.26.2     │ 3.12.1     │ 3.1.58     │ 0.26.0 -                  │ Yes        │
│ 0.26.1     │ 3.12.1     │ 3.1.58     │ 0.26.0 -                  │ Yes        │
│ 0.26.0     │ 3.12.1     │ 3.1.58     │ 0.26.0 -                  │ Yes        │
└────────────┴────────────┴────────────┴───────────────────────────┴────────────┘

which is quite nicer!

@agriyakhetarpal
Copy link
Member Author

The only thing left for me to do here is check with cibuildwheel and see if this works there or if we need something extra. I'll do that later in the day today and mark this ready for review.

@agriyakhetarpal agriyakhetarpal changed the title Add a JSON output for pyodide xbuildenv search Add a JSON output for pyodide xbuildenv search, better tabular output Sep 12, 2024
@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Sep 12, 2024

I've tried this with cibuildwheel, and it should work fine. The build procedure in cibuildwheel has a bit complicated for me to understand (not the Pyodide builds but setting the identifiers correctly, still on that), but the idea is that I'm successfully able to retrieve a list of compatible Pyodide xbuildenv versions within Python as a list of strings: ["0.27.0a2", "0.26.2", "0.26.1", "0.26.0"] from the stdout stream from running this command.

This should help set up a base for developing wheels against arbitrary Pyodide versions in cibuildwheel. After we bump the constraints to use a newer version of pyodide-build in cibuildwheel/resources/constraints-pyodide312.txt, we'll be able to retrieve the above output from the JSON, and pass a non-alpha Pyodide xbuildenv version to the install_xbuildenv() method in cibuildwheel/pyodide.py. @ryanking13, I do have a question about this approach, though: how should we decide on what Pyodide versions to support? i.e., if Pyodide v0.X.Y would be available through pyodide-build v0.M.N, what would be the suitable values of X, Y, M, and N? In real-world scenarios, the question would be whether pyodide-build v0.35.0 in the future can support, say, a much older Pyodide 0.26.0? In case we implement some breaking changes in pyodide-build, we would have to bump the minimum version of Pyodide that we are compatible with. Do we have a policy on that, thus far? If not, should we implement one?

In any case, this is ready for review – if we need a better output from this command, we can always implement improvements to it in subsequent PRs and versions.

@agriyakhetarpal agriyakhetarpal marked this pull request as ready for review September 12, 2024 15:05
@agriyakhetarpal
Copy link
Member Author

Also, the context manager fixes our problem that was present in pyodide/pyodide#4814.

@ryanking13
Copy link
Member

I do have a question about this approach, though: how should we decide on what Pyodide versions to support? i.e., if Pyodide v0.X.Y would be available through pyodide-build v0.M.N, what would be the suitable values of X, Y, M, and N? In real-world scenarios, the question would be whether pyodide-build v0.35.0 in the future can support, say, a much older Pyodide 0.26.0? In case we implement some breaking changes in pyodide-build, we would have to bump the minimum version of Pyodide that we are compatible with. Do we have a policy on that, thus far? If not, should we implement one?

My goal is to prevent breaking changes as much as possible, but yes, some changes are inevitable.

If any breaking change happens, I'll basically update the min_pyodide_build_version, and max_pyodide_build_version in the metadata file, to specify the compatibility of each Pyodide version and pyodide-build.

The problem at this point is that when people say they want to build a package that targets an older version of Pyodide, they don't know which version of pyodide-build to install. What people can do now is to install pyodide-build first, check for compatible versions with pyodide xbuildenv search, and then install pyodide-build for those versions again, which is not a very good user experience. I haven't come up with a clean solution to this yet, so if you have any ideas, please share it with us.

Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

Thanks! I left some comment.

CHANGELOG.md Outdated Show resolved Hide resolved
pyodide_build/cli/xbuildenv.py Outdated Show resolved Hide resolved
@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Sep 16, 2024

What people can do now is to install pyodide-build first, check for compatible versions with pyodide xbuildenv search, and then install pyodide-build for those versions again, which is not a very good user experience. I haven't come up with a clean solution to this yet, so if you have any ideas, please share it with us.

I have a few thoughts in mind:

  • One way could be to backport the xbuildenv search feature to older pyodide-build versions. This is doable, but it comes at the cost of doing it and backporting it, which is tedious.
  • We can't really support more than one Python version in Pyodide, because doing so would be a burden on maintenance.
  • We could add the generated table of supported Pyodide versions against a pyodide-build version to the README here and continually update it after every release and/or breaking change. This would benefit users because they would know where to look for what version to install directly without having to download and execute code.
  • Another aspect is that a lot of people are going to use cibuildwheel to build Pyodide binary wheels in conjunction with already established, cibuildwheel-enabled, CD jobs, and once we have a reasonable wheel hosting solution, upload the wheel artifacts there. So, it would make sense for cibuildwheel to do this internally and suggest a few Pyodide versions to the users against the constrained pyodide-build version that would be present there (which is why, this PR).

I think it all boils down to the Pyodide version support we can offer in a realistic manner. For example, we should make it explicit that someone should not use, say, Pyodide version 0.21 in 2024 (just an example). The Pyodide version is also influenced by the versions of the packages present in it, because they would have to be compatible with the CPython version in Pyodide. This symbiotic relationship between the Pyodide, CPython, and the Emscripten versions is what makes Pyodide work as a full-fledged distribution, but it doesn't offer guarantees on release and support cycles. With the Scientific Python ecosystem having adopted SPEC 0000 for bumping minimum versions of dependencies and minimum bounds of CPython, if we have our own consensus on a minimum supported Pyodide version in the release cycle (please let me know if we have one already!), we can better gauge a solution for pyodide-build's support for a minimum Pyodide version.

Either way, a Markdown table in the README could be added (from the third point).

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Sep 16, 2024

The integration tests seem to fail to run on my fork, but that is not the case here. I triggered them here so that there are no regressions later on.

@agriyakhetarpal agriyakhetarpal mentioned this pull request Sep 17, 2024
@ryanking13
Copy link
Member

We could add the generated table of supported Pyodide versions against a pyodide-build version to the README here and continually update it after every release and/or breaking change. This would benefit users because they would know where to look for what version to install directly without having to download and execute code.

Yes, I think this is a most doable solution that does not increase the maintenance burden. We can probably also provide a file that specifies the maximum compatible pyodide-build version for each version of Pyodide (actually, our metadata file can already be used for that feature).

if we have our own consensus on a minimum supported Pyodide version in the release cycle

Actually, we don't have any support for older Pyodide versions. If something breaks in older Pyodide, we just ask them to "update the Pyodide" (or pyodide-build). But yes, as now pyodide-build is unvendored and people start to build packages with cibuildwheel, we need to consider it...

Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

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

Thanks! I left some minor comments.

pyodide_build/views.py Outdated Show resolved Hide resolved
pyodide_build/views.py Outdated Show resolved Hide resolved
Comment on lines 29 to 31
@pytest.fixture()
def is_valid_json():
def _is_valid_json(json_str):
Copy link
Member

Choose a reason for hiding this comment

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

Question: Why is it defined as a fixture? Doesn't calling is_valid_json work?

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 was using it in the test invocation, which meant that it needed to be a fixture for pytest to recognise it. But we don't need that, necessarily. In a9f9a61, I've removed the fixture and called it directly, as you suggest.

@agriyakhetarpal
Copy link
Member Author

We could add the generated table of supported Pyodide versions against a pyodide-build version to the README here and continually update it after every release and/or breaking change. This would benefit users because they would know where to look for what version to install directly without having to download and execute code.

Yes, I think this is a most doable solution that does not increase the maintenance burden. We can probably also provide a file that specifies the maximum compatible pyodide-build version for each version of Pyodide (actually, our metadata file can already be used for that feature).

if we have our own consensus on a minimum supported Pyodide version in the release cycle

Actually, we don't have any support for older Pyodide versions. If something breaks in older Pyodide, we just ask them to "update the Pyodide" (or pyodide-build). But yes, as now pyodide-build is unvendored and people start to build packages with cibuildwheel, we need to consider it...

Thanks for your thoughts on this. Yes, linking to the metadata file makes sense, too (it's just that a formatted table is more readable with the same data). I guess we should open a new issue about this, because it is also tied in with, say, including multiple versions of packages in the same Pyodide version (useful for interactive documentation across versions). I guess I need to write a detailed RFC issue about that prospect...

@agriyakhetarpal
Copy link
Member Author

I'll put up a reminder about thinking more about this, I'll hopefully write something after PyCon this week. In other words, I guess this is ready to merge, now. Thank you for the review!

@agriyakhetarpal agriyakhetarpal merged commit 6662c28 into pyodide:main Sep 18, 2024
6 checks passed
@agriyakhetarpal agriyakhetarpal deleted the feat/xbuildenv-search-json-option branch September 18, 2024 10:27
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.

pyodide xbuildenv search --json option
2 participants