Skip to content

Move book tests to github runners#4629

Merged
fingolfin merged 8 commits intooscar-system:masterfrom
aaruni96:ak96/book-tests-to-gh-runner
Feb 26, 2025
Merged

Move book tests to github runners#4629
fingolfin merged 8 commits intooscar-system:masterfrom
aaruni96:ak96/book-tests-to-gh-runner

Conversation

@aaruni96
Copy link
Copy Markdown
Member

PR to check how long the book tests take on GH runners.

@aaruni96
Copy link
Copy Markdown
Member Author

We have this line in the CI.yml

runs-on: ["${{ matrix.os }}", "${{ matrix.group == 'short' && 'high-memory' || 'normal-memory'}}", RPTU]

It adds the label "RPTU" to ensure everything runs on RPTU runners, and also, labels a job "high memory" or "normal memory", depending on whether its the short tests, or everything else. So, I have no easy way to move just the book tests to github runners (alternatively, we get rid of trickery in this line, but then we must figure out another hack to get back the behaviour of short tests on high memory runners).

Should I break out the book tests into another job, like we do with doctests?

@benlorenz

@aaruni96 aaruni96 force-pushed the ak96/book-tests-to-gh-runner branch from afaf5d3 to 894ef6f Compare February 21, 2025 17:03
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.41%. Comparing base (31167d4) to head (7ca030c).
Report is 22 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4629      +/-   ##
==========================================
+ Coverage   84.11%   84.41%   +0.29%     
==========================================
  Files         674      674              
  Lines       89781    89880      +99     
==========================================
+ Hits        75522    75874     +352     
+ Misses      14259    14006     -253     

see 61 files with indirect coverage changes

@benlorenz
Copy link
Copy Markdown
Member

Should I break out the book tests into another job, like we do with doctests?

Yes, I think that should be ok. It also looks nicer on the overview with the graph.

Comment thread .github/workflows/CI.yml Outdated
- 'pre'
- 'nightly'
group:
- 'book'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You don't need to set the group here, and could instead hardcode it below. This would also remove some duplication in the job title booktest (1.10, book).
But no high priority

@fingolfin
Copy link
Copy Markdown
Member

I am happy with this being moved to another job set, bit just oit of curiosity, couldn't you also just do this:

runs-on: ["${{ matrix.os }}", "${{ matrix.group == 'short' && 'high-memory' || 'normal-memory'}}", "${{ matrix.group == 'book' || 'RPTU'}}]

@fingolfin fingolfin added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Feb 22, 2025
@aaruni96
Copy link
Copy Markdown
Member Author

We can't do that because if it gets labeled "normal-memory" or "high-memory", then github will not pick them up either.

Jobs will be assigned to runners which match all of the labels on the job. Github runners are only labelled with the OS, so, ubuntu-latest. Any additional label guarantees it doesn't run on github runner.

Label RPTU is only for future proofing, in case, in the future, we have a self hosted runner for Oscar which isn't hosted in RPTU.

Comment thread .github/workflows/CI.yml Outdated
@fingolfin
Copy link
Copy Markdown
Member

This is marked as a draft, so what exactly is to be done or decided?

@aaruni96
Copy link
Copy Markdown
Member Author

what is to be decided?

This PR started off as a check to how much slower book tests would be on the github runners v/s rptu runners. (Answer: ~10 minutes). Its not yet decided (AFAIK) whether or not you actually greenlight this change yet.

Co-authored-by: Max Horn <max@quendi.de>
@lgoettgens lgoettgens added the CI label Feb 24, 2025
@fingolfin
Copy link
Copy Markdown
Member

If they are just 10 minutes slower, I'd say, let's do this. And then we monitor. And if necessary it is easy enough to undo this, too.

@aaruni96 aaruni96 marked this pull request as ready for review February 25, 2025 10:59
Copy link
Copy Markdown
Member

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

This just needs someone (@fingolfin or @benlorenz) to adapt the list of required CI jobs

@lgoettgens lgoettgens added the backport 1.3.x backport for release branch 1.3 label Feb 26, 2025
@fingolfin fingolfin merged commit 84f2ccf into oscar-system:master Feb 26, 2025
@aaruni96 aaruni96 mentioned this pull request Feb 27, 2025
11 tasks
aaruni96 added a commit that referenced this pull request Feb 27, 2025
(cherry picked from commit 84f2ccf)
Signed-off-by: Aaruni Kaushik <aaruni@edufor.me>
aaruni96 added a commit that referenced this pull request Feb 28, 2025
Backports for 1.3.0 : 

Add DOI of the book #4654
Add attribute for more detailed info on tunable sections #4636
Move book tests to github runners #4629
Update Nemo to 0.49 #4645
fix a NiceMonomorphism for GAP constructed in Oscar #4640
Polyhedral: fix hash for halfspace and related types #4600
Cleanup some code related to dim(I) == -inf checks #4571
Fix bug in G4-flux computation of big model #4647
Add more missing hash functions #4638
Document how changelog maintenance works #4579
Update changelog #4667
@aaruni96 aaruni96 removed the backport 1.3.x backport for release branch 1.3 label Mar 7, 2025
fieker pushed a commit that referenced this pull request Mar 17, 2025
@aaruni96 aaruni96 deleted the ak96/book-tests-to-gh-runner branch April 23, 2026 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants