Skip to content

Support new bsk SWIG compilation using JSON#6

Open
ReeceHumphreys wants to merge 4 commits intodevelopfrom
feature/cmake-json
Open

Support new bsk SWIG compilation using JSON#6
ReeceHumphreys wants to merge 4 commits intodevelopfrom
feature/cmake-json

Conversation

@ReeceHumphreys
Copy link
Copy Markdown
Collaborator

@ReeceHumphreys ReeceHumphreys commented Apr 10, 2026

AVSLab/basilisk#1343 introduced some changes to the way we compile SWIG modules in bsk. This PR makes the corresponding changes for the sdk to read this new output format. This ensures the SDK works with the 2.11 release.

Additionally, this PR introduces nightly testing so we can catch this incompatible changes with the SDK rapidly by building the develop head of the sdk against the develop head of bsk each day.

@ReeceHumphreys ReeceHumphreys self-assigned this Apr 10, 2026
Copy link
Copy Markdown
Contributor

@schaubh schaubh left a comment

Choose a reason for hiding this comment

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

Looks like the test of the example file is failing right now. A few more tweaks required. Nice work overall.


- uses: actions/setup-python@v6
with:
python-version: "3.13"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't we be using 3.14 now?

@schaubh
Copy link
Copy Markdown
Contributor

schaubh commented Apr 10, 2026

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a9284d8cfe

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

steps:
- uses: actions/checkout@v5
with:
submodules: true
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Fetch Basilisk develop before nightly build

This checkout configuration only initializes external/basilisk at the submodule SHA pinned in the SDK repo, and there is no later step that fetches/checks out Basilisk's develop tip. As a result, the nightly job will keep testing the same Basilisk revision and miss breaking changes introduced after that pinned commit, which defeats the stated purpose of a nightly compatibility check against Basilisk develop.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ReeceHumphreys , this might be the issue with the tests not working?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Unrelated to the tests failing. They were failing because the CI tried to fetch the bsk wheel but the latest one it was able to fetch does not contain the latest changes. I've added logic now to get from bsk develop.

Automatically publishing a nightly bsk wheel from the bsk repo could make things like this simpler in the future. Might be worth thinking about.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You are thinking of building nightly wheels from develop and putting them where? Can't do PyPi, we'd run out of space. Can we put them on GitHub and simply replace it each night to avoid using too much disk space? Could be handy to have a Linux build of latest develop for the AI researchers as well.

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