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

Upgrade awkward-cpp to 27 #4345

Closed

Conversation

pyodide-pr-bot
Copy link
Contributor

@pyodide-pr-bot pyodide-pr-bot commented Dec 17, 2023

This PR was created by a bot. Pinging @agoose77, @jpivarski to check this

@ryanking13
Copy link
Member

Hi, @agoose77, I guess you made this bot? This seems like a great way to help us keep our package versions up to date, thanks a lot!

However, I have a few questions and concerns so I'm leaving a comment.

  1. The name pyodide-pr-bot gives people the impression that this bot is a project managed by pyodide. If you want to take ownership of this bot, you might want to rename it. Otherwise, we can make a repository inside the Pyodide organization for you and this project.

  2. Updating a package version is often not very trivial. It requires some manual work, including version conflicts between packages and the need to add patches. So if this bot starts to create one PR for each package in Pyodide, it would start to accumulate a huge number of PRs that don't work. I'm worried that other important PRs might get overshadowed by the package update PR and not get checked properly.

@agoose77
Copy link
Contributor

agoose77 commented Dec 19, 2023

Hi @ryanking13 — I appreciate the thoughtful reply! 😄

  1. This PR was authored from https://github.com/agoose77/pyodide-pr-bot, which I'm migrating to pyodide-pr-bot/pyodide-pr-bot. I chose a name that would be amenable to moving into pyodide's care, but that wasn't clear from the start — thanks for the feedback. I'm not wedded to this at all. I am happy to maintain things under a new name, but if you would find it useful to live under your org, you're more than welcome (I can maintain it there!).
  2. This is opt-in, for packages where it makes sense. The idea is that individuals can register their packages at https://github.com/agoose77/pyodide-pr-bot, which will then open draft PRs that bump the version. It will also ping the associated maintainers (defined in e.g. https://github.com/agoose77/pyodide-pr-bot/blob/main/packages/awkward-cpp.json) to notify them of the TODO.
    • I completely recognise the concerns about creating broken PRs. This solves a problem for us at awkward — it can be hard to keep track of all the various todo items for releases when one has to wait for things to e.g. become available on PyPI, and also takes out a small amount of the busywork. In our case, awkward-cpp almost never changes in any way besides the compiled sources, meaning that it's highly likely that new versions just need version bumps (modulo any compile issues under emscripten).
    • It would be easier to make this less obtrusive if we had the full conda-forge feedstock model. Is the idea of feedstocks something that pyodide has considered?

The tool is not perfect — it would be nice to directly permit the pinged maintainers to edit the PR, without needing to create a new PR upstream on https://github.com/pyodide-pr-bot/pyodide. If pyodide isn't interested in a conda-forge model, then we could handle that over at https://github.com/pyodide-pr-bot probably.

@agoose77
Copy link
Contributor

@ryanking13 finally — in case it is not clear, I'm not interested in causing any problems for maintaining pyodide. I think this tool has a use (it does for awkward!) but we can make it a scikit-hep tool only if you're feeling concerned about the maintenance burden incurred by unvetted package updates. So, keep me posted — I'll make changes accordingly :)

@pyodide-pr-bot pyodide-pr-bot marked this pull request as ready for review December 19, 2023 11:38
@ryanking13
Copy link
Member

I chose a name that would be amenable to moving into pyodide's care, but that wasn't clear from the start — thanks for the feedback. I'm not wedded to this at all. I am happy to maintain things under a new name, but if you would find it useful to live under your org, you're more than welcome (I can maintain it there!).

Great! Then, if other core dev are okay, we will transfer it to Pyodide organization and give maintainer permissions to you. WYTH @rth, @hoodmane?

This is opt-in, for packages where it makes sense. The idea is that individuals can register their packages at https://github.com/agoose77/pyodide-pr-bot,

That would work. Thanks for the clarification! I also like the idea of pinging the authors (though we don't have an authors section in meta.yaml like conda for now, but we can add it)

It would be easier to make this less obtrusive if we had the full conda-forge feedstock model. Is the idea of feedstocks something that pyodide has considered?

I think for now, the direction where Pyodide is headed is to let package maintainers build their own packages. So, we are focused on supporting out-of-tree builds, and support building Pyodide wheels in cibuildwheel. But the conda-forge model can also be considered, I think in the future.

in case it is not clear, I'm not interested in causing any problems for maintaining pyodide. I think this tool has a use (it does for awkward!) but we can make it a scikit-hep tool only if you're feeling concerned about the maintenance burden incurred by unvetted package updates. So, keep me posted — I'll make changes accordingly :)

Thanks! I'm sorry if my comment intimidated you (I'm not a native English speaker, so my tone may not have been appropriate). I am so happy and very grateful that others are making these great contributions.

@agoose77
Copy link
Contributor

Thanks! I'm sorry if my comment intimidated you

No, not at all! It's rather a reflection of my own caution; I once opened N (>25) PRs on conda-forge feedstocks to add a centralised maintainer without consulting people individually. It was an automation-driven mess, and taught me a useful lesson about keeping people in the loop when it comes to automation and CI/CD!

@agoose77
Copy link
Contributor

@ryanking13 could we open a new issue to continue this discussion? I think it would be useful to figure out any workflow changes that might be necessary. Right now, the infrastructure creates a PR from a CI-owned repo (https://github.com/pyodide-pr-bot/pyodide) which most maintainers of packages won't have push access to. This requires them to make a new PR on that repo if they want to tweak the PR that the "bot" creates.

@hoodmane
Copy link
Member

Thanks for creating the ci bot @agoose77! It would indeed be good to discuss it further in a separate issue.

I have no objection to having a pyodide/pyodide-ci-bot, hopefully @agoose77 is willing to maintain it here. Presumably it should gate on being enabled in a meta.yaml key so we can turn it off with a pr for packages where it is failing?

And we may have to think about how this fits into @ryanking13's plan to do something more sane with the package builds...

@ryanking13 ryanking13 mentioned this pull request Dec 23, 2023
1 task
@ryanking13
Copy link
Member

Thanks! I created an issue to do more discussion about this #4350

@ryanking13
Copy link
Member

Oh, I just noticed that this PR rearranges the key order in the meta.yaml file, making the about section go to the top, could you fix it so it does not change the order?

@jpivarski
Copy link

This is good discussion about the bot, but that has moved to #4350.

Meanwhile, the awkward-cpp version has also updated to 28, which is on PR #4380, so this PR can be closed. (I can't do that.)

@agoose77
Copy link
Contributor

So, right now only the pyodide admins and the @pyodide-pr-bot can close this PR. We should probably add a probot so that we can ask @pyodide-pr-bot to close the PR on behalf of the maintainers.

The longer-term solution is to follow the feedstocks model, where we can safely assign privileges to the maintainers.

I'll make a note to close this using the PR account.

@hoodmane
Copy link
Member

Thanks @agoose77!

@hoodmane hoodmane closed this Jan 19, 2024
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