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

CI: Add FLAMEGPU_SEATBELTS=OFF CI via matrix include: #1138

Merged
merged 2 commits into from
Nov 1, 2023
Merged

Conversation

ptheywood
Copy link
Member

@ptheywood ptheywood commented Oct 27, 2023

CI: Add FLAMEGPU_SEATBELTS=OFF CI via matrix include:

Using include is very verbose, but wanted to not multiply the exisiting matrix by 2, or trial and error expand the exclude region.

Switching to dynamic build matrices via a 2 step job would be nice but likely significant effort

Closes #1136

Todo:

  • Add atleast one windows and one linux beltsoff ci build
  • Fix CI YAML to be valid according to the GitHub Workflow web editor Deferred to a separate issue CI Matrix Syntax #1145
  • Make standalone examples resepect all matrix options
    • Optionally, reduce the number of standalone example builds to save total time, its mostly testing the CMake so multiple compiler versions etc is probably unneccesary

@ptheywood
Copy link
Member Author

This should fail currently, until #1137 is resolved.

@ptheywood
Copy link
Member Author

It seems that my existing, working use of complex objects within a matrix is not strictly correct/supported even though it works.

In the web editor, the cudacxx objects are reported as an error.

Matrix options must only contain primitive values

I might have to figure out the "correct" way to do this in general (I.e. where using a specific cuda version means a specific value of cuda_arch/hostcxx should be used).

It might be that we have to do a fully include based matrix for that to be possible, which will quickly become incredibly verbose for wheel generation...

@Robadob
Copy link
Member

Robadob commented Oct 28, 2023

Would a simpler compromise be to configure ALL standalone example builds to be SEATBELTS=OFF?

That would force a main library build, but wouldn't catch all device API template code (only going to fully get that with the test suite).

It's possibly a suitable compromise, if release flow ensures tests are built SEATBELTS=OFF too

@ptheywood
Copy link
Member Author

That would be an option to quickly get a FLAMEGPU_SEATBELTS=OFF build covered, but the matrix yaml I've used everywhere for the cuda compiler would still be invalid.

From some github issues / feature requests it seems that lists can be used as matrix elements, so potentially could use something like:

` - compilerstuff: ["12.3", "50", gcc-12, ubuntu-22.04]

instead of

- cuda: "12.3"
  cuda_arch: "50"
  hostcxx: gcc-12
  os: ubuntu-22.04

Though its a bit less readable / easy to make mistakes.

Thankfully I did map matrix values to env vars where they are used, so the changes should only need to be prior to jobs starting. I'll mock up the matrix stuff in a separate repo when I have some time.

ptheywood added a commit that referenced this pull request Oct 30, 2023
This is a short-term / quick way to get a beltsoff build into CI, suggested by @Robadob as short term part of #1138.

This will be replaced by a 'correct' version in the future (as our current matrix is technically unsupported, although it mostly works, and is not expliucitly documented as such other than in the web workflow editor
ptheywood added a commit that referenced this pull request Oct 30, 2023
This is a short-term / quick way to get a beltsoff build into CI, suggested by @Robadob as short term part of #1138.

This will be replaced by a 'correct' version in the future (as our current matrix is technically unsupported, although it mostly works, and is not expliucitly documented as such other than in the web workflow editor
…orkflows

Using a number of exclude statemements, per OS the CI matrix should be:

+ 3 release console builds
+ 2 release vis builds
+ 1 beltsoff build

The matrix YAML is not techincally supported by Github actions according to the web editor
We should probably find a compliant way to do this, but it works so a problem for the future.

The individual example uses the same (relevant) options as the main configuration.
@ptheywood
Copy link
Member Author

This is now ready for review, assuming that CI passes.

There are now 6 builds for regular Ubuntu and Windows CIs:

  • Latest CUDA 12, Release, Console
  • Latest CUDA 12, Release, Vis
  • Latest CUDA 12, beltsoff, Console
  • Latest CUDA 11, Release, Console
  • Latest CUDA 11, Release, Vis
  • Oldest CUDA 11, Release, Console

We could probably drop the CUDA 11.8 Release Vis build, as the vis shouldn't be significantly tied to the CUDA version, but it might be worth keeping.

Adding a single GLM build might be worthwhile, but with the current matrix syntax that will be a lot of additional exclude statements.

@ptheywood ptheywood marked this pull request as ready for review November 1, 2023 11:39
Copy link
Member

@Robadob Robadob left a comment

Choose a reason for hiding this comment

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

The changes look sensible. Can merge assuming CI passes.

@Robadob Robadob added the CI label Nov 1, 2023
@Robadob
Copy link
Member

Robadob commented Nov 1, 2023

Adding a single GLM build might be worthwhile, but with the current matrix syntax that will be a lot of additional exclude statements.

Useful sure, but not a priority whilst GLM remains experimental and not fully supported by Python.

@ptheywood ptheywood merged commit e52194a into master Nov 1, 2023
20 checks passed
@ptheywood ptheywood deleted the ci-seatbelts branch November 1, 2023 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI FLAMEGPU_SEATBELTS=OFF
2 participants