Skip to content

Conversation

@user202729
Copy link
Contributor

@user202729 user202729 commented Mar 16, 2025

Something I notice while working on #39717.
Previously, the clause "Otherwise, …" doesn't hold.

πŸ“ Checklist

βŒ› Dependencies

@github-actions
Copy link

github-actions bot commented Mar 16, 2025

Documentation preview for this PR (built with commit 2b51201; changes) is ready! πŸŽ‰
This preview will update shortly after each push to this PR.

Copy link
Member

@vincentmacri vincentmacri left a comment

Choose a reason for hiding this comment

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

LGTM.

By the way, since you're a very active contributor with a lot of open PRs, could you avoid merging develop into all your branches at once, especially branches you aren't actively working on or haven't gotten around to implementing requested changes yet? I think you recently merged develop into all of your open PRs and now the CI has a big queue to get through.

For things you're actively working on or hope to merge soon this is fine, but maybe hold off on merging develop into your draft PRs unless you're about to work on them since a full CI run takes about an hour and is triggered on every commit.

@vincentmacri
Copy link
Member

Feel free to set to positive review after CI finishes unless there is a (relevant) failure.

@user202729
Copy link
Contributor Author

user202729 commented Sep 11, 2025

Hm… that's a valid concern.

However on my side the advantage of merging everything quickly is that otherwise, when I do a git checkout old-branch β†’ git checkout develop, the build system notice that everything's timestamps have changed and rebuild everything. (I'm using meson, just not ccache.)

I suppose there's a workaround of merging them locally, but only push them X days after release for sufficiently large X.

Of course, the ideal solution is to address the root cause, namely to implement a proper prioritization system for GitHub actions.

https://github.com/orgs/community/discussions/135274
https://github.com/orgs/community/discussions/44087

Looking at a few discussions there's no good way to manually change workflow priority, so manually triggered workflow is probably needed. Difficult to implement though.

Edit: Actually it's probably not that difficult to implement, e.g. do something such as make a job re-schedule and fail itself if certain preconditions are not satisfied. Now it's just whether someone volunteers to do this…

@vincentmacri
Copy link
Member

vincentmacri commented Sep 11, 2025

Yeah, it would be nice if GitHub had better tools for managing workflow priority but I don't think it does.

I suppose there's a workaround of merging them locally, but only push them X days after release for sufficiently large X.

That would probably work fine. And for smaller PRs (like this one) where realistically nothing is going to break then I don't think you even need to bother merging develop unless there are merge conflicts to resolve.

@user202729
Copy link
Contributor Author

I have a script that automatically merge all of them automatically, so thinking about which pull request to (not) merge would actually be more difficult.

Actually 10.8.beta2 is 4 days ago. Surely X=4 is fine?

On the GitHub actions issue: I think the mechanisms for implementing fail/rerun etc. are all there, it's just someone need to write the code.

@vincentmacri
Copy link
Member

I have a script that automatically merge all of them automatically, so thinking about which pull request to (not) merge would actually be more difficult.

Most of the time you probably don't need to merge develop at all unless there is a merge conflict. Maybe just don't push merge commits until you have a non-trivial commit to push? If the merge goes through no problem then there's not really an issue of backing up your work since the work was just running git merge.

Actually 10.8.beta2 is 4 days ago. Surely X=4 is fine?

I don't think it's so much about when you merge it, but rather about how many PRs you're merging develop into at once. You have 51 open PRs right now. Running all CI checks takes a few CPU hours, so putting 51 all at once into the queue is a lot. Maybe if you stagger it and have your script only update some fraction of the PRs when you run it?

Another solution might be to have the CI cancel some tests if other tests already failed. For example, if the regular build is failing then only test a handful of Linux distros in the Meson workflow instead of all of them. Or don't run the PDF doc build if the HTML doc build failed since the HTML one is more stable and if it breaks then something is probably going to break with the PDF build too. Or combine workflows to do some things sequentially and avoid recompiling as much.

You can configure the CI to run on PRs on your own repo by the way, if you want to test that CI passes on your own repo before you're ready to make a PR.

@user202729
Copy link
Contributor Author

user202729 commented Sep 12, 2025

actually there's a much easier to implement solution, just don't run the CI on clean merge commit if the previous commit is already tested. With a few downsides such as vbraun have a few more false positives to deal with, but likely very rare.

nonetheless, I should clarify a few things.

Another solution might be to have the CI cancel some tests if other tests already failed.

This is a non-issue, since the CI often doesn't fail anyway. Except on the draft pull requests, but draft pull requests make a very small fraction of my pull requests.

if you want to test that CI passes on your own repo

This is also a non-issue, I know that the CI most likely pass as well.

Maybe merging locally is actually fine. One disadvantage I can think of is that if someone suggest a change on GitHub using the "suggest change" feature and I accept that, I have divergent branches, but this is likely uncommon.

combine workflows to do some things sequentially and avoid recompiling as much

This is… not really an issue. If you look at https://github.com/sagemath/sage/actions/runs/17671866157/job/50225193038, build takes only 6m 20s compared to https://github.com/sagemath/sage/actions/runs/17643520919/job/50135862758?pr=39718 which takes 22m 17s, the latter is because the compiler cache exists (expand the "Compiler cache" tab).

It still takes 6m instead of something like 30s because compiler cache works by using ccache, which does not cache cython compilations (.pyx β†’ .c), only (.c β†’ .so) is cached.

When Docker was used (test-new), the whole file system was backed up and restored, so we save the cython overhead. See how test-new at vbraun@8081c7c takes only 13m, despite the overhead of using make instead of meson. That feature is dropped in #39641, but someone could implement that as well.

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 16, 2025
sagemathgh-39718: Add a note regarding element containment testing
    
Something I notice while working on
sagemath#39717.
Previously, the clause "Otherwise, …" doesn't hold.

### πŸ“ Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [x] I have updated the documentation and checked the documentation
preview. (see https://doc-pr-39718--sagemath.netlify.app/html/en/themati
c_tutorials/coercion_and_categories.html#equality-and-element-
containment)

### βŒ› Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39718
Reported by: user202729
Reviewer(s): Vincent Macri
@vbraun vbraun merged commit 1b43bdb into sagemath:develop Sep 21, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants