221.Add force checkout and compile functionality #236
Conversation
…eckout-and-compile -also fix decorator function in `run_fremake.py`
…rce-checkout-and-compile
…rce-checkout-and-compile
…rce-checkout-and-compile
…rce-checkout-and-compile
- code was repeated if `--force-compile` was used
- code was repeated if `--force-checkout`, `--force-compile` or `--force-dockerfile` was used
- code would be duplicated if `--force-dockerfile` option is used
…consistency
…rce-checkout-and-compile
…rce-checkout-and-compile
|
Looks fairly well thought out to me here- any tests coming? maybe add quick desc of what the PR adds in plain english when you can! |
Oh wait! I can totally add test to test_run_fremake since that's in there already. (I can also add to the compile test I have going in a PR and work with Avery to add tests for checkout). Maybe the compile test can be merged in first - then I can add to it to test this functionality. Also good idea on the description, I think I meant to add to it when I took it out of draft but then forgot |
|
@ilaflott Thanks for taking a look in your spare time btw |
|
i'd help resolve these conflicts, approve+merge whenever you're ready. |
Thanks @ilaflott. I'll work on merge conflicts and probably ping you when I think everything's good |
…rce-checkout-and-compile - resolve merge conflicts
- these changes will be done in another issue
…rce-checkout-and-compile
- instead of passing fremake object to run function, just pass compile script path to run - this allows an already created compile script to also run in parallel (if not being created or if no `force-compile` is used)
|
@ilaflott @uramirez8707 @rem1776 Ooook, had to iron out some things that I was doing that I actually didn't want to be doing, but I think this PR should be ok for reviews/tests now 🤞 |
| if run: | ||
| if force_checkout: | ||
| # Remove previous checkout | ||
| print("\nRemoving previously checkout script and checked out source code") |
There was a problem hiding this comment.
I think rather than removing it you should back up the src and exec directory. This what bronx ouputs
WARNING: Since you've specified --force-compile (-F), existing compile and environment scripts have been renamed using timestamp '20250130.085825'
New compile and environment scripts will be created...
You can remove old scripts later by typing:
rm -f /gpfs/f5/gfdl_f/scratch/Uriel.Ramirez/SPEAR_experiments_Q_2025.01-beta1/SPEAR_Q2025.01-beta1_nonsymMOM6_exec/ncrc5.intel23-classic-repro-openmp/exec/compile_SPEAR_Q2025.01-beta1_nonsymMOM6_exec.csh.20250130.085825
rm -f /gpfs/f5/gfdl_f/scratch/Uriel.Ramirez/SPEAR_experiments_Q_2025.01-beta1/SPEAR_Q2025.01-beta1_nonsymMOM6_exec/ncrc5.intel23-classic-repro-openmp/exec/env.cshrc.20250130.085825
| ## If combined yaml exists, note message of its existence | ||
| ## If combined yaml does not exist, combine model, compile, and platform yamls | ||
| combined = Path(f"combined-{name}.yaml") | ||
| full_combined = cy.combined_compile_existcheck(combined,yml,platform,target) |
There was a problem hiding this comment.
I think if the combined yaml exists, it should combined them again and compare them and output a warning if they are not the same and suggest the --force* options
For example, if you change something in your yaml and you forget to delete the combined yaml, fre-canopy is going to use the combined-yaml that already exist and won't do what you expect (I have done this sooooo many times :/)
This is what bronx outpust:
WARNING: The existing checkout script '/gpfs/f5/gfdl_f/scratch/Uriel.Ramirez/SPEAR_experiments_Q_2025.01-beta1/SPEAR_Q2025.01-beta1_nonsymMOM6_exec/src/checkout.csh' doesn't match checkout instructions in the XML file!
If you need a fresh checkout, please rerun the fremake with the --force-checkout (-f) option.
There was a problem hiding this comment.
I agree with this 200%.
This is making me wonder though, why not go one step further and just not even write the combined yaml to a file?
I will admit, I never really understood why we combine the yamls and write it out in the first place, so maybe there's a better reason then I'm seeing.
But if the current fremake yamls (platform/compile/etc) are what we are going to be modifying and using in place of xmls, the combined yamls don't really need to exist as files, they can stay in memory. Otherwise, they just kinda get in the way of whatever yaml files the user specifies.
…rce-checkout-and-compile
| os.chmod(src_dir+"/checkout.sh", 0o744) | ||
| print(" Checkout script created in "+ src_dir + "/checkout.sh \n") | ||
|
|
||
| return fre_checkout |
There was a problem hiding this comment.
just curious, is this returned for a reason?
Describe your changes
This PR adds
force-checkoutforce-compileforce-dockerfileThese make it easier to edit scripts in between steps. Previously, if steps were run to make scripts, then re-run, there would just be a print statement that the script existed already. However, if the user wanted to change something, let's say in the checkout script, then re-run to execute it, that functionality was not there. They would've had to edit something in their configuration and start over.
Issue ticket number and link (if applicable)
#221
Checklist before requesting a review