Skip to content

New CI Workflow#405

Merged
dellaert merged 16 commits intomasterfrom
feature/simplifiedCI
Feb 2, 2026
Merged

New CI Workflow#405
dellaert merged 16 commits intomasterfrom
feature/simplifiedCI

Conversation

@dellaert
Copy link
Member

@dellaert dellaert commented Feb 1, 2026

Currently the Mac and Linux CI are failing (on #403 and #404 ) because I fixed a bug on gtsam. So I would have to build new Docker images and a new brew thingy. That's very burdensome.

In the meantime, the Python CI builds the whole thing anyway.

So, this PR removes the Mac and Linux CI and expands the Python CI to also run the C++ tests. The resulting workflow, which builds gtsam for the three tested platforms, is now common-ci.yml. I also added a new document to reflect this new situation.

@dellaert dellaert requested a review from varunagrawal February 1, 2026 15:31
@dellaert
Copy link
Member Author

dellaert commented Feb 1, 2026

Remaining TODO (in a later PR, after I merged open PRs):

  • Switch to latest platform versions (24.04 and MacOS 15)
  • Have both Linux and Mac exercise the install prefix, for simplified workflow

Copy link
Contributor

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

I don't see anything glaring from looking at the code, but CI is failing so I can look into that.

@varunagrawal
Copy link
Contributor

PS I triggered the docker container builds manually so you are unblocked on other PRs.

@dellaert
Copy link
Member Author

dellaert commented Feb 1, 2026

No, no worry about it. I'm dealing with those issues. I might close this PR if these issues are solved in a different PR. I'd rather you not add more PRs and take up limited CI resources at this point - racing against the clock.

@dellaert
Copy link
Member Author

dellaert commented Feb 2, 2026

@varunagrawal I take this back. I can't figure out for the life of me why we would have errors here. We should not. ChatGPT led me down a rabbithole of gcc-specific "false positives", but clang also has issues.

Since it does work on Mac, I do suspect it is somehow a difficulty with compatibility between GTSAM and GTDynamics on Linux. On my Linux machine, I have the same issues with both gcc and clang.

If you can make sense of this or debug it, that would be a lifesaver.

@varunagrawal
Copy link
Contributor

@varunagrawal I take this back. I can't figure out for the life of me why we would have errors here. We should not. ChatGPT led me down a rabbithole of gcc-specific "false positives", but clang also has issues.

Since it does work on Mac, I do suspect it is somehow a difficulty with compatibility between GTSAM and GTDynamics on Linux. On my Linux machine, I have the same issues with both gcc and clang.

If you can make sense of this or debug it, that would be a lifesaver.

I already figured it out and made PR #407 targeting this branch to fix it. :)

@dellaert
Copy link
Member Author

dellaert commented Feb 2, 2026

Ow, shoot, CI has the same errors I saw on my machine… could be alignment- but not issue on Mac

@dellaert
Copy link
Member Author

dellaert commented Feb 2, 2026

I turned off the margin native in both repos, let's see what that does overnight.

@dellaert
Copy link
Member Author

dellaert commented Feb 2, 2026

@varunagrawal hold off on edits, trying something

@varunagrawal
Copy link
Contributor

@varunagrawal hold off on edits, trying something

Cool, I've been debugging in a docker container in the meantime.

@dellaert
Copy link
Member Author

dellaert commented Feb 2, 2026

Okay, @varunagrawal, I fired up another CI. If you think you still need to make changes, please do so via PR targeting this branch and make it small. I like where I ended up after substantial editing - if it works :-) I put all the parallel makes to using -j2 because before it was crashing. You somehow reverted that in the other PR.

@dellaert
Copy link
Member Author

dellaert commented Feb 2, 2026

I also thought I had increased memory to avoid crashes, but that seems gone too.

@dellaert
Copy link
Member Author

dellaert commented Feb 2, 2026

@varunagrawal this seems to work, so I'll merge (after CI succeeds completely), and then merge master into the other PRs. If there is another urgent fix it can be made in #403 after I merge in master.

@dellaert dellaert merged commit e89b225 into master Feb 2, 2026
@dellaert dellaert deleted the feature/simplifiedCI branch February 2, 2026 17:57
@dellaert
Copy link
Member Author

dellaert commented Feb 2, 2026

Merged! Thanks for your help, @varunagrawal !

@varunagrawal
Copy link
Contributor

I definitely didn't touch the number of processes or the swap space recently, maybe 3 years ago?

Did you manage to isolate the issue? The march-native setting seems to be the most likely culprit here.

@varunagrawal
Copy link
Contributor

I'm still confused why it works in the Docker container and not on bare metal. Is it something to do with the underlying servers of GitHub actions?

@dellaert
Copy link
Member Author

dellaert commented Feb 2, 2026

@varunagrawal I think you did touch it when you removed my common-ci.yml. I like it better this way, so thank you. But the number of threads was altered in the process, I think.

Now everything works. We just have to make sure -march native is consistent between gtsam and gtdynamics. In the current CI, we're now exercising native and non-native on all platforms.

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.

2 participants