fre make documentation updates part 1#735
fre make documentation updates part 1#735laurenchilutti wants to merge 12 commits intoNOAA-GFDL:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #735 +/- ##
==========================================
+ Coverage 82.76% 84.10% +1.33%
==========================================
Files 68 70 +2
Lines 4525 4780 +255
==========================================
+ Hits 3745 4020 +275
+ Misses 780 760 -20
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
…file which is in restructured text format.
…e for the checkout as they are uniquely different. Updated documentation.
…th baremetal and container platforms by replacing an else with an elif. Update comments and hellp message text in fremake.py. Update fre make tests to work with the updated -j/--njobs argument that I replaced with -mj/--makejobs and -gj/--gitjobs.
…eadability, creating a custom class inheriting from click.Group, so that the fre make subtools are listed in the order they are defined in the file, rather than alphabetical order. This gives the user a better idea of the order you would run these commands. Fixing error in string spanning multiple lines.
|
This is ready for a review. I plan to make more updates in future PRs, but want to keep the changes small enough in each PR so that they are easy to review. |
mlee03
left a comment
There was a problem hiding this comment.
Round 1 of reviews, what do you think of the made suggestions?
|
Should be ready to merge - glossary can always be improved as we go. |
mlee03
left a comment
There was a problem hiding this comment.
Tiny changes, other than that, what's here is a good start for the chatbot~
|
ready for another review! |
| target | ||
| The term refers to the ``fre make`` argument which defines compiler options to turn on during compilation. | ||
| ``fre-cli`` requires either ``prod``, ``repro``, or ``debug`` followed by any number of supplementary options | ||
| separated by a ``-`` such as ``openmp`` and ``lto``. For example: ``repro-openmp``, ``prod-openmp-lto`` |
There was a problem hiding this comment.
one more change. officially seeing it, it looks like a run-on.
The term refers to the fre make argument which specifies compiler options for model compilation. The value of the string must start with prod, repro, or debug followed by optional, supplementary tags that are separated by -. for example, a target can have value of repro-openmp, prod-openmp-lto. See [link] for more details @singhd789 these targets are defined somwhere in fre-cli right?
There was a problem hiding this comment.
Hm, I think they only place they are maybe discussed is here:
Lines 144 to 149 in ad432b1
and honestly...it could be better and moved elsewhere (like in fre make docs - especially because they're not really used much in fre pp)
There was a problem hiding this comment.
how about linking this class? https://github.com/NOAA-GFDL/fre-cli/blob/main/fre/make/gfdlfremake/targetfre.py
There was a problem hiding this comment.
I'm curious if there can be any other targets passed that we don't define in the code. At the moment, I think if a user were to pass other supplementary targets besides openmp and lto, it would still create the necessary fre make scripts (even it if was a nonexistent target which is not the best).
If we don't want this to happen (and just stick to/control the targets/supplementary targets), maybe we list the potential combinations in click.Choice (like this: https://github.com/NOAA-GFDL/fre-cli/blob/main/fre/yamltools/freyamltools.py#L19) and say "for target choices, see fre make all --help". This might be adding too much though, maybe we can do that in the next iteration!
There was a problem hiding this comment.
For now, I personally like giving the target / supplementary target names here
Describe your changes
Updated
fremake.py:run_fremake_script.py,test_run_fremake_builds.py, andtest_run_fremake.pyallcommand a short help description, as the full description gets cutoff at an awkward point when you dofre make --help. The full help description will be displayed withfre make all --helpUpdated docs:
Issue ticket number and link (if applicable)
Checklist before requesting a review