-
Notifications
You must be signed in to change notification settings - Fork 18
Fix Cubit STEP import for MagnetSetFromGeometry
#266
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
Conversation
|
For some reason, |
I think this is the problem: we now (and for some time now) install parastell when we build and push the Before we were doing pip install, the pytest only had the local code available. We probably need to pip install in the ci tests before running tests. |
Ensure that newest code is installed for CI testing
|
Hopefully #267 will fix this… |
|
I changed #267 to be a PR into this branch.... I reran the test, but not sure if that actually picked up your changes. |
Force install of updated code for testing in CI
|
I guess that didn't help |
|
Based on GitHub's documentation, if I'm understanding correctly, the commit checked out by |
gonuke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to understand how this testing is setup
.github/workflows/ci.yml
Outdated
| . /opt/etc/bashrc | ||
| pip install --no-deps . | ||
| /opt/Coreform-Cubit-2025.8/bin/rlm_activate --login ${EMAIL_SECRET} ${PASSWORD_SECRET} | ||
| cd /opt/parastell/tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait... why do we go to /opt/parastell/tests to do our testing? That isn't where we checkout the repo... maybe removing that will help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does the repo get checked out? The documentation is unclear to me Seems to be checked out in the PWD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, the ParaStell imports in the test suite are not relative, so I'm not sure the correct version of ParaStell will be used
gonuke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more CI testing questions
|
|
||
| - name: Populate environment and run tests | ||
| run: | | ||
| . /opt/etc/bashrc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also... I can't figure out what is in /opt/etc/bashrc that is so important?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That may be an artifact of when we put ParaStell on PYTHONPATH explicitly. Maybe unnecessary at this point, unless that's still necessary for Cubit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command also activates the appropriate conda environment (see the Dockerfile)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably the conda activation that is important 👍🏻
|
We're also still having the issue that we're using up Cubit license seats during CI. When tests fail, |
|
@gonuke looks like everything is working now |
gonuke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there was actually a bug :) glad testing helped you find it!
|
Yeah, there was a new bug introduced by this PR that the test wasn't reaching before |
A bug prevented Cubit from being able to import STEP files from a
MagnetSetFromGeometryobject. Specifically, inbuild_cubit_model, we create a new magnet STEP file to ensure that magnet volumes are ordered and grouped appropriately. This STEP export previously reset thegeometry_fileattribute before the file was actually exported, which called theMagnetSetFromGeometrysetter, which then tries to import said STEP file into CadQuery to resolve the geometry. This led to an error where the STEP file could not be found (understandably).This PR makes the following changes:
MagnetSet.export_stepmethod sets astep_pathattribute to differentiate from the original user-setgeometry_fileattributeMagnetSet.import_geom_cubitmethod uses thestep_pathattribute if it’s been set, otherwise uses the originalgeometry_fileattribute. TheStellarator.build_cubit_modelmethod will leverage the former workflow, to ensure volumes are ordered appropriately. For meshing, etc.,geometry_filewill sufficeMagnetSetFromGeometrywill now resolve the geometry only ifgeometry_fileis a STEP file (CUB5 is also allowed)import_dirfrom allcubit_utilsfunctions, since a full path infilenameis really all that’s necessary