Update ci: resolve incompatible environments for damask and fenics
#262
Conversation
… and environment-notebooks-fenics.yml
… exclude_damask and exclude_fenics
|
With I found on a local env on my client, that an ad-hoc solution for this is to go from Also, this will be solved via #256. |
|
With On a local client, I get the error |
There was a problem hiding this comment.
Since exclude_X excludes "Not X, but the other thing", let's rename them exclude_for_X. Otherwise lgtm. I added a number of suggestions for shortening action calls, and extracting common environment elements to a shared file then merging this to reduce maintenance headache, but these are merely suggestions and I can live without them -- file names are the only sticking point for me.
| - matplotlib =3.8.0 | ||
| - numpy =1.26.3 | ||
| - pyiron =0.5.0 | ||
| - pyiron-data =0.0.26 | ||
| - python-damask =3.0.0a8 | ||
| - pyvista =0.42.3 | ||
| - scipy =1.11.4 | ||
| - seaborn =0.13.1 | ||
| - scikit-image =0.22.0 | ||
| - papermill | ||
| - jupyter |
There was a problem hiding this comment.
Since most of the damask and fenics envs are shared, I would suggest showing this explicitly with environment-notebooks-shared/damask/fenics.yml files, and then merge shared+damask and shared+fenics. This will reduce the overhead of keeping all the shared versions sync'd. Some of this might even move into the core (non-notebook) env, such that the tests use merges of core+shared+damask, core+shared+fenics
There was a problem hiding this comment.
Check this centralized action for a tool to throw in the CI scripts for merging envs like this
| @@ -26,7 +45,7 @@ jobs: | |||
| shell: bash -l {0} | |||
| run: | | |||
| pip install --no-deps . | |||
| mamba env update --name test --file .ci_support/environment-notebooks.yml | |||
| mamba env update --name test --file .ci_support/environment-notebooks-fenics.yml | |||
There was a problem hiding this comment.
And similar for shortening the fenics job
Co-authored-by: Liam Huber <liam.huber@gmail.com>
|
Now I'm confused (at 11pm after two glasses of wine): Following @liamhuber's suggestion I made this commit, which erased the conda environment creation for damask, and for fenics there's just an 'update'. By specifying the environment file it creates a conda environment? I don't quite understand how GitHub knows how to handle the environment. |
|
Was there a decision how we want to handle this issue in the case of https://github.com/pyiron/docker-stacks ? So far we had one Docker container for |
That's a good point. I don't have a good idea right now, but yeah why not have two |
I opened an issue in |
|
@liamhuber: If you are fine with that, I'd suggest to first just make the ci here somehow running again. Your requested changes are super legit and @samwaseda was so kind to integrate them. But because there are all the time new errors popping up, I don't want to do too many things at once in this PR. For anything related to using stuff from our centralized actions and making other optimizations (like improved environment creation), I opened another issue to keep track of this. |
There was a problem hiding this comment.
Now I'm confused (at 11pm after two glasses of wine): Following @liamhuber's suggestion I made this commit, which erased the conda environment creation for damask, and for fenics there's just an 'update'. By specifying the environment file it creates a conda environment?
I think I see what you're saying -- after the commit in question the notebooks jobs don't even run. I peeked at the action log, and it claims You have an error in your yaml syntax on line 9, which is just the jobs: line. If you look at the remaining file though, in the build_damask: job, my suggestion starting with - uses: pyiron/actions/cached-mamba@main has inconsistent indentation! I think this is what's causing the error. I suggest just trying again with the damned thing formatted correctly 😂
I don't quite understand how GitHub knows how to handle the environment.
Each jobs: item runs independently on its own runner, which means its own filesystem and its own environment. So, the cached-mamba action from pyiron/actions is just cloning in the repo, merging two (or more, or one) env files into a new file on that job's filesystem, and then using that new env file to make a conda env. The damask and fenics jobs can do this independently on their own runners just fine as long as they're in separate jobs (which they are!)
I'm not 100% sure I've understood your misunderstanding, so just ping me if I'm not being clear 😝
liamhuber: If you are fine with that, I'd suggest to first just make the ci here somehow running again. Your requested changes are super legit and samwaseda was so kind to integrate them. But because there are all the time new errors popping up, I don't want to do too many things at once in this PR.
For anything related to using stuff from our centralized actions and making other optimizations (like improved environment creation), I opened another issue to keep track of this.
The file names are fixed, so I'm switching this to "approved". However, I do still strongly suggest you implement my other suggestions right here in this PR. My reasoning is: the remaining suggestions -- using the centralized CI to set up the envs, and extracting commonalities in env files to turn two envs files into three -- do not modify existing code in the repo, but rather new code being added in this PR. In particular, my suggestions shorten the amount of lines added and avoid code duplication.
I like the new issue! pyiron/actions is set up so that the re-usable workflows make some assumptions about repo structure, but the individual actions are (or should be!) agnostic and usable everywhere. I.e. there's no need to make a total conversion in a single shot. Slipping in a -uses: pyiron/actions/cached-mamba@actions-1.1.0b in this PR is not fundamentally any different than - uses: actions/checkout@v2.
(Note: my suggestion was bad, we should target a locked-in version actions-1.1.0b and not just main)
|
The ci throws an error when running the notebook tests, I think the relevant part is: TypeError Traceback (most recent call last)
Cell In[14], line 1
----> 1 grid = pr.continuum.damask.Grid.via_voronoi_tessellation(box_size=1.0e-5,grid_dim=grids,num_grains=grains)
TypeError: GridFactory.via_voronoi_tessellation() got an unexpected keyword argument 'grid_dim'However, I don't really understand why, because when looking at I tried to reproduce the error like so:
Then, I start a jupyter session and open the notebook |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
The failing tests were a result of som inconsitencies originating from a previously merged PR. Fixing the notebooks accordingly did the trick. I merged this now because @usaikia is in urgent need. I'm working right away on relying more heavily on centraliced actions in #263 . |
As described in this PR, dependencies of packages
fenicsanddamaskare incompatible right now.With my changes here, we can at least support exclusively the one or the other packages, but not both simultaneously in the same environment, unfortunately.
Running all test for both packages independently is maybe not an elegant solution, but a pragmatic one. Reverting them, once the dependencies are compatible is super easy and straightforward.
Dropping
fenics-support at all would be kind of a last resort to solve this...