Render each case study separately - #117
Conversation
iantaylor-NOAA
left a comment
There was a problem hiding this comment.
@e-perl-NOAA, thanks for working on this. I can do a more detailed review later, but if the GOA pollock error is causing none of the case studies to render, then we might need to rethink the approach. My hope was that whichever case studies successfully render could be published while producing a warning or notification listing any case studies that failed.
|
@iantaylor-NOAA, the GOA pollock one as well as the other ones are all failing, they mostly have the same error about Petrale error: Hake error: |
|
@e-perl-NOAA, I can try to clean up these errors to get them working again. That way we can focus on refining the workflow instead of having everything fail. Do you think it's better to push fixes to this branch or a separate one? Pushing here avoids requiring this to rebase, but also makes changes more complex to review. |
|
I'm fine with changes being made to this branch to get the case studies working. Really we just need one working to show proof of concept. |
… with fallback to previous version for failed ones; update readme with success/failures; have full render do everything except case studies
… case studies fail
Fix a bug introduced in last commit
Co-authored-by: megumi.oshima@noaa.gov Co-authored-by: adrianne.wilson@noaa.gov
bf7b4d7 to
4043e94
Compare
|
Some of the case studies are rendering which is good enough for a test of the deploy which I was able to do and create an artifact here |
|
@e-perl-NOAA, the opakapaka, petrale, and sardine qmd files are now all running through the rendering step but run into errors with "Upload rendered artifact" like the following, copied from https://github.com/NOAA-FIMS/case-studies/actions/runs/30409569678/job/90442603805. Any ideas on what needs to change? |
|
In the Lines 1 to 8 in 61d9d7f So those aren't actually deployed/used for the website despite being rendered. I don't do enough with the case studies to know if they should be rendered and included in the |
|
@e-perl-NOAA and @kellijohnson-NOAA, I think it makes sense that the final published site only includes the case studies that we humans choose to include (via _quarto.yml as you say), where the choice of what we include will evolve over time. However, I think it's really valuable to see which of the case studies are passing and failing, whether we choose to include them in the published site or not. The |
…tifact Added a check for published case studies before uploading artifacts.
Retain existing files during deployment to gh-pages.
|
@iantaylor-NOAA I made some changes so that there aren't failures when it doesn't upload artifacts for case studies not included in the |
- name to fleet - fleet_name to fleet - replace set_all_estimable() and set_all_random() with set_estimation_types()
|
@e-perl-NOAA, this is perfect, thank you! @kellijohnson-NOAA, pacific hake was the only one that was still failing, and I've attempted some changes in 16303fe but am not confident about how I've changed |
|
Looks like my changes to pacific hake now have everything passing in the Render Case Studies GHA. I'm not sure if there's a way to test the Deploy Case Studies GHA before merging this PR since I think it depends on push to main, but as far as I'm concerned, we can merge this any time and revise from there. It doesn't seem like a big deal with we temporarily break the case-studies github.io site for a little while at this stage of FIMS development. I see that @kellijohnson-NOAA is out today, so probably better to give her final say on when this is ready. Thanks for all your work on this @e-perl-NOAA! |
|
I glanced this over quickly. The only thing that is slightly concerning to me, not a blocker though, is the addition of code that is not functionized to the R folder. Everything still works because the default is to not run it, i.e., if (FALSE) {}, but that part of the code should really be in the petrale.qmd file in a code chunk. @iantaylor-NOAA feel free to merge in now and change that later though. |
|
@iantaylor-NOAA, if you merge this in and get the message that the deploy workflow has failed, please let me know and I'll work on trying to fix it. Anything in terms of deploying to a github site is hard to test because you often don't want the test deployed to the main site. I found a way around this for the fims website but not all site need a preview feature. However if you would find it useful to have a preview workflow/feature in this repo too, let me know. |
- thanks @kellijohnson-NOAA for the nudge
|
Thanks @kellijohnson-NOAA. I modified (in 579db39) the |
@kellijohnson-NOAA While this is not exactly the logic you outlined in #113, it does have some similarities and makes it so that you don't have to manually update workflow names for specific case studies
Right now none of the case studies are rendering because of the following error, is that error the same one we saw in code club yesterday due to changes in FIMS? If so, I think I missed what the fix was...though looking back it's changing
fleet_nametofleet?1. Render Case Studies workflow
content/**changes2. Deploy Case Studies workflow
3. Render and Deploy Full Site workflow
4. Removes previous build and deploy case studies workflow