221.Add force-checkout 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
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.
There was a problem hiding this comment.
I agree with both of you, this is great input!
@rem1776 - At first, I was trying to load the yamls and just have/use the dictionary it's parsed as, and then update/add to dictionaries in order to dump a combined yaml file with all the info needed. BUT, the main issue I was seeing here was from the reusable variables.
The yaml won't even load (it will give "undefined alias" issue) if there is a key that uses a reusable variable that isn't defined in fre_properties, these being name, platform, and target, and those also aren't defined in fre_properties because they can change (since they're click options that are passed in).
I think this is related to your confusion on why we even create these combined yamls. If there's a better way, I'm definitely up for it though.
…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?
singhd789
changed the title
singhd789
changed the title
force-checkout functionality
Describe your changes
This PR adds
force-checkoutPreviously, 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