LFRic Apps apply upgrade macros script (Part 1) #38

james-bruten-mo · 2024-10-15T10:41:13Z

This PR is to add the main part of the upgrade macro script for applying macros to LFRic. As described in #37 this work will be done in 2 commits, of which this is the first. This code will read through versions.py files, searching for newly added macros, and combine these macros where necessary. The 2nd part will add the rose commands to apply the upgrade macros as well as adding unit tests. Some development history and testing examples can also be seen in lfric_apps:#139 including a description of the overall approach on the details page. A PR for the working practices also exists.

This PR adds:

a main and parse args section
introduces the ApplyMacros class which is where the majority of the code is. This contains:
- a section for sourcing required lfric_core and jules working copies (as these contain required metadata)
- a section for reading and combining new upgrade macros (the majority)
some initial functions outside of the above class which provide some general functionality

The script can be tested by getting the following 2 branches:

fcm:lfric_apps.x_br/pkg/jamesbruten/vn1.1_macro_pkg
fcm:lfric.x_br/dev/jamesbruten/core1.1_core_macro_branch

And then running apply_macros.py vn1.1_t139 -a /path/to/apps -c /path/to/core. This upgrade macro is present in lfric_atm, transport, gungho and components/driver. After running the script, it should be removed from gungho and driver, combined with the existing macro in atm and transport and added to any metadata that imports any of those 4 (most of them in fact!).

jennyhickson

Lots of trivial comments but generally looking good.

I realised towards the end that I was finding "meta_dir" as the main thing being used as a reference not very intuitive and had to remind myself what that is refering to. Possibly because its both the path to a versions.py and a path to rose-meta.conf and also used as a general key for each app/science section (because those are really one and the same). I don't have an easy suggestion, but wonder if it could be named differently to help with that?

lfric_macros/apply_macros.py

james-bruten-mo

Thanks Jenny. Addressed all your inline comments and retested to make sure it still works (as far as it goes in part 1). Only thing I haven't changed is the meta_dir variable as I'm not sure what to do. Happy to have a sit down and come up with something together if think changing would be good.

james-bruten-mo · 2024-11-26T18:35:00Z

lfric_macros/apply_macros.py

+            - str, stdout of find command looking for versions.py files
+        """
+
+        command = f"find {path} -name versions.py"


Ok, I've taken the opportunity to learn how os.walk works! It's remarkably easy and definitely a better solution than a find command. Cheers

james-bruten-mo · 2024-11-27T08:50:49Z

lfric_macros/apply_macros.py

+                # skip blank lines
+                continue
+            if match_python_import(line):
+                started_imports = True


Yeah, it's not very robust so I've changed how this works

james-bruten-mo · 2024-11-27T08:54:56Z

lfric_macros/apply_macros.py

+    def write_python_imports(self, meta_dir):
+        """
+        Write out all required python module imports at the top of a versions.py
+        file. Read imports from self.python_imports


Updated the doctring and modified the code as above. The import statements are now written at the top of the current import statements

james-bruten-mo · 2024-11-27T09:09:46Z

lfric_macros/apply_macros.py

+            "Jules sources."
+        )
+
+    def read_meta_imports(self, meta_dir, flag="import"):


Have set this to just return a list, rather than trying to be too clever about which flag it's got. It's probably marginally less efficient but not really noticeable and a lot more readable. Changed the calls as required in this point - the other bits are in part 2.

jennyhickson

Thanks. One more comment and over to Sam!

lfric_macros/apply_macros.py

t00sa

Looks good. I've added some comments in line and I've got a couple of general questions, mostly related to error handling and reporting.

What is the intended behaviour if the script fails during execution? Is it acceptable for it to generate empty and/or partial version.py files and to delegate the process of cleaning up and fixing to the user? Or should the script always fail safe and leave the versions.py files intact?

Is it worth add a top-level exception handler which catches and formats RuntimeErrors? If theses can be triggered by the user, then it would be better to generate a user-friendly message rather than displaying a traceback.

I've suggested using the logging module rather than printing things out. My feeling is that it is better to use logging calls than print because it allows the amount of output to be tuned at runtime, but this is partly because I prefer commands that don't produce a lot of output.

t00sa · 2024-11-27T11:34:11Z

lfric_macros/apply_macros.py

+    # return the found path
+    for line in result.stdout.split("\n"):
+        if line.startswith("Working Copy Root Path"):
+            return line.split(":", 1)[1].strip()


Is it worth raising an exception after the loop in case the working copy doesn't match? Or is safe to assume that fcm will always provide a valid string if the return code is zero?

t00sa · 2024-11-27T11:41:06Z

lfric_macros/apply_macros.py

+        file_raw = f.readlines()
+    file_parsed = []
+    for line in file_raw:
+        if len(line.strip().strip("\n")) > 0:


We've discussed this and I don't think the double strip is necessary:

>>> s = " abc \n" >>> s ' abc \n' >>> s.strip() 'abc' >>>

I also wonder if it might be clearer to say:

Suggested change

if len(line.strip().strip("\n")) > 0:

if line.strip() != "":

If you're just filtering non-blank lines?

t00sa · 2024-11-27T13:16:58Z

lfric_macros/apply_macros.py

+            ticket_details = re.search(r"Upgrade .* (#\d+) by (\S+.*)", macro)
+            try:
+                self.ticket_number = ticket_details.group(1)
+                self.author = ticket_details.group(2).rstrip('".')
+                self.author = self.author.strip("<>")


How about doing all the work in the regular expression match?

Suggested change

ticket_details = re.search(r"Upgrade .* (#\d+) by (\S+.*)", macro)

try:

self.ticket_number = ticket_details.group(1)

self.author = ticket_details.group(2).rstrip('".')

self.author = self.author.strip("<>")

ticket_details = re.search(r"Upgrade .* (#\d+) by <?([^\.]+)>?\.?", macro)

try:

self.ticket_number = ticket_details.group(1)

self.author = ticket_details.group(2)

t00sa · 2024-11-27T13:28:22Z

lfric_macros/apply_macros.py

+
+        filepath = os.path.join(meta_dir, "versions.py")
+
+        with open(filepath, "w") as f:


This would benefit from some error handling. Best option would be to write to a temporary file in the same directory and rename once the file has been written but a simple check to make sure the versions file isn't empty might be sufficient...

Having done as we discussed (I think), it fails as the target file already exists. Which makes sense but I don't know if I've not done things as expected. So instead of os.link, I've just gone with os.rename() to remove the temp file

t00sa · 2024-11-27T13:35:20Z

lfric_macros/apply_macros.py

+                    except AttributeError:
+                        raise RuntimeError(
+                            "Couldn't find an after tag in the macro:\n"
+                            f"{macro}"
+                        )


Should raise from the previous exception:

Suggested change

except AttributeError:

raise RuntimeError(

"Couldn't find an after tag in the macro:\n"

f"{macro}"

)

except AttributeError as exc:

raise RuntimeError(

"Couldn't find an after tag in the macro:\n"

f"{macro}"

) from exc

Done that elsewhere, clearly missed. Updated now

t00sa · 2024-11-27T14:21:39Z

lfric_macros/apply_macros.py

+        with open(fpath, "w") as f:
+            for line in versions_file:
+                f.write(line.strip("\n") + "\n")


As with the previous write, worth thinking about writing this to a temporary copy and renaming once the write is successful

Done, as above. Have also modified the one with append below

t00sa · 2024-11-27T14:47:37Z

lfric_macros/apply_macros.py

+        except KeyError:
+            # Jules Shared directories will produce a key error - these are
+            # guaranteed to not import anything
+            imports = []


Is it clearer to return an empty list immediately?

Suggested change

imports = []

return []

yeah definitely

t00sa · 2024-11-27T14:56:30Z

lfric_macros/apply_macros.py

+            print(
+                "[INFO] Pre-processing macros in",
+                self.parse_application_section(meta_dir),
+            )


As a general point, have you considered using logging module functions instead of printing to stdout?

I hadn't, but I think I prefer printing, particularly as this will regularly be used by people not familiar with it. My feeling is that for those, it's better to give some positive feedback that it's actually doing something - particularly as the 2nd PR for this introduces the stuff that takes a bit of time!

And there are only 4/5 print statements so we can easily come back to this later

t00sa · 2024-11-27T15:17:04Z

lfric_macros/apply_macros.py

+    # if the class name doesn't conform to the expected vnXX.Y_tTTTT naming
+    # convention raise an error
+    class_name = args.tag.replace(".", "")
+    if not re.match(CLASS_NAME_REGEX, class_name):
+        raise RuntimeError(
+            f"The tag '{args.tag}' does not conform to the "
+            "'vnXX.Y_tTTTT' naming scheme. Please modify and rerun."
+        )


It might be better to move this into a function that could be passed in to paser.add_argument() in parse_args() to allow all the command line option error handling to be managed by argpse, e.g.

def check_tag(opt): class_name = opt.replace(".", "") if not re.match(CLASS_NAME_REGEX, class_name): raise argparse.ArgumentTypeError( f"The tag '{opt}' does not conform to the " "'vnXX.Y_tTTTT' naming scheme. Please modify and rerun." ) return opt ... parser.add_argument( "tag", type=check_tag, metavar="after-tag", help="The After Tag of the upgrade macro being upgraded to.", )

Well that's neat!

t00sa · 2024-11-27T15:27:41Z

lfric_macros/apply_macros.py

+    macro_object.preprocess_macros()
+
+    # Clean up temporary directories
+    for repo, directory in macro_object.temp_dirs.items():


Have you considered using tempfile.TemporaryDirectory() to create a top-level directory and letting the library handle the clean-up when it goes out of scope?

Yes, but there is a possibility that the script gets the temporary copy of core and updates apps in there, in which case I don't want it to get deleted, so think I need to do this manually?

james-bruten-mo

Thanks Sam. Have addressed all your inline comments. I've left a note about using the logging module. I think having the prints is useful as otherwise it can feel like it's hanging. Happy to discuss that further though - I am probably not using the logging module perfectly either.

Only thing I haven't done is implement a custom error handling. There are a number of raise Exceptions with custom error messages - is it these you want changing? Or is it to catch a runtime error that I haven't got an exception for? In which case surely we want the stack trace?

james-bruten-mo · 2024-11-27T16:09:51Z

lfric_macros/apply_macros.py

+    # return the found path
+    for line in result.stdout.split("\n"):
+        if line.startswith("Working Copy Root Path"):
+            return line.split(":", 1)[1].strip()


james-bruten-mo · 2024-11-27T16:10:12Z

lfric_macros/apply_macros.py

+        file_raw = f.readlines()
+    file_parsed = []
+    for line in file_raw:
+        if len(line.strip().strip("\n")) > 0:


james-bruten-mo · 2024-11-27T16:29:15Z

lfric_macros/apply_macros.py

+
+        filepath = os.path.join(meta_dir, "versions.py")
+
+        with open(filepath, "w") as f:


Having done as we discussed (I think), it fails as the target file already exists. Which makes sense but I don't know if I've not done things as expected. So instead of os.link, I've just gone with os.rename() to remove the temp file

james-bruten-mo · 2024-11-27T16:29:56Z

lfric_macros/apply_macros.py

+                    except AttributeError:
+                        raise RuntimeError(
+                            "Couldn't find an after tag in the macro:\n"
+                            f"{macro}"
+                        )


Done that elsewhere, clearly missed. Updated now

james-bruten-mo · 2024-11-27T16:31:24Z

lfric_macros/apply_macros.py

+        # First grep for lines starting with import=
+        # No imports if it doesn't return anything
+        result = run_command(f"grep -E '^ *{flag}=' {meta_file}", shell=True)
+        if not result.stdout:
+            return []
+
+        imports = []
+        with open(meta_file, "r") as f:


Have just deleted the grep - I don't think we need to be that clever about it. These aren't large files, nor is it a performance dependent script

james-bruten-mo · 2024-11-27T16:49:50Z

lfric_macros/apply_macros.py

+    macro_object.preprocess_macros()
+
+    # Clean up temporary directories
+    for repo, directory in macro_object.temp_dirs.items():


Yes, but there is a possibility that the script gets the temporary copy of core and updates apps in there, in which case I don't want it to get deleted, so think I need to do this manually?

james-bruten-mo · 2024-11-27T16:50:14Z

lfric_macros/apply_macros.py

+            ticket_details = re.search(r"Upgrade .* (#\d+) by (\S+.*)", macro)
+            try:
+                self.ticket_number = ticket_details.group(1)
+                self.author = ticket_details.group(2).rstrip('".')
+                self.author = self.author.strip("<>")


james-bruten-mo · 2024-11-27T17:00:26Z

lfric_macros/apply_macros.py

+        with open(fpath, "w") as f:
+            for line in versions_file:
+                f.write(line.strip("\n") + "\n")


Done, as above. Have also modified the one with append below

james-bruten-mo · 2024-11-27T18:43:09Z

lfric_macros/apply_macros.py

+            print(
+                "[INFO] Pre-processing macros in",
+                self.parse_application_section(meta_dir),
+            )


I hadn't, but I think I prefer printing, particularly as this will regularly be used by people not familiar with it. My feeling is that for those, it's better to give some positive feedback that it's actually doing something - particularly as the 2nd PR for this introduces the stuff that takes a bit of time!

james-bruten-mo · 2024-11-27T18:43:55Z

lfric_macros/apply_macros.py

+            print(
+                "[INFO] Pre-processing macros in",
+                self.parse_application_section(meta_dir),
+            )


And there are only 4/5 print statements so we can easily come back to this later

t00sa

Almost there. I've added a few suggested changes.

lfric_macros/apply_macros.py

Co-authored-by: Sam Clarke-Green <74185251+t00sa@users.noreply.github.com>

james-bruten-mo · 2024-12-02T12:07:28Z

Almost there. I've added a few suggested changes.

Thanks Sam, have merged in all those suggestions directly

t00sa

Looks good. Approved.

t00sa · 2024-12-02T12:44:54Z

Over @jennyhickson for approval

add apply_macros

fc50861

james-bruten-mo mentioned this pull request Oct 15, 2024

LFRic Apps apply upgrade macros script (Part 2) #39

Merged

minor tweaks to macros script

8f6063b

james-bruten-mo requested a review from r-sharp November 7, 2024 14:55

run black

1dba2be

james-bruten-mo requested review from jennyhickson and t00sa and removed request for r-sharp November 11, 2024 09:23

james-bruten-mo added 2 commits November 21, 2024 11:25

update regex

03e8f59

add opt for different class name and tag

b55e546

jennyhickson reviewed Nov 26, 2024

View reviewed changes

review updates

7a7b207

james-bruten-mo commented Nov 27, 2024

View reviewed changes

james-bruten-mo requested a review from jennyhickson November 27, 2024 09:16

jennyhickson reviewed Nov 27, 2024

View reviewed changes

lfric_macros/apply_macros.py Show resolved Hide resolved

t00sa reviewed Nov 27, 2024

View reviewed changes

james-bruten-mo added 2 commits November 28, 2024 08:57

review comments

f00cb4f

run black

59a6bfd

james-bruten-mo commented Nov 28, 2024

View reviewed changes

james-bruten-mo requested a review from t00sa November 28, 2024 09:03

t00sa reviewed Dec 2, 2024

View reviewed changes

lfric_macros/apply_macros.py Outdated Show resolved Hide resolved

lfric_macros/apply_macros.py Outdated Show resolved Hide resolved

lfric_macros/apply_macros.py Outdated Show resolved Hide resolved

james-bruten-mo and others added 3 commits December 2, 2024 12:05

Update lfric_macros/apply_macros.py

8742f5e

Co-authored-by: Sam Clarke-Green <74185251+t00sa@users.noreply.github.com>

Update lfric_macros/apply_macros.py

b52c0a1

Co-authored-by: Sam Clarke-Green <74185251+t00sa@users.noreply.github.com>

Update lfric_macros/apply_macros.py

643358f

Co-authored-by: Sam Clarke-Green <74185251+t00sa@users.noreply.github.com>

t00sa approved these changes Dec 2, 2024

View reviewed changes

james-bruten-mo requested a review from jennyhickson December 2, 2024 12:45

jennyhickson approved these changes Dec 2, 2024

View reviewed changes

jennyhickson merged commit ce9a8aa into main Dec 2, 2024
1 check passed

james-bruten-mo deleted the apps_upgrade_macros_1 branch September 1, 2025 15:43


		filepath = os.path.join(meta_dir, "versions.py")

		with open(filepath, "w") as f:

LFRic Apps apply upgrade macros script (Part 1) #38

LFRic Apps apply upgrade macros script (Part 1) #38

Uh oh!

Conversation

james-bruten-mo commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jennyhickson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

james-bruten-mo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jennyhickson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

t00sa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

james-bruten-mo commented Oct 15, 2024 •

edited

Loading