Updated exit code for DRC regression #78

mkkassem · 2022-11-24T01:47:07Z

Fixes #<64>

proppy · 2022-11-25T01:34:11Z

rules/klayout/drc/testing/run_regression.py

-
-from sympy import arg
+import pandas as pd
+import logging



consider using https://docs.pytest.org/en/7.2.x/

@proppy That's a big change right now. We can move to it later.

then can we file an issue to track that work?

@proppy Yes please do.

proppy · 2022-11-25T01:34:30Z

rules/klayout/drc/testing/run_regression.py

    t0 = time.time()
    marker_gen = []
-    rules =[]
-    ly = 0
+    rules      = []


can you make sure this follow https://peps.python.org/pep-0008/ ?

@proppy Will check how we can change that

https://flake8.pycqa.org/en/latest/ and https://github.com/psf/black tools come to mind.

@proppy Will take that into account.

proppy · 2022-11-25T01:35:36Z

rules/klayout/drc/testing/run_regression.py

    name_ext = str(rule_deck_path).replace(".drc","").split("/")[-1]
-    os.system(f"mkdir run_{x}_{name_ext}")
+    check_call(f"mkdir run_{x}_{name_ext}", shell=True)


use https://docs.python.org/3/library/os.html#os.makedirs

@proppy Will do.

proppy · 2022-11-25T01:37:40Z

rules/klayout/drc/testing/run_regression.py

    name_ext = str(rule_deck_path).replace(".drc","").split("/")[-1]
-    os.system(f"mkdir run_{x}_{name_ext}")
+    check_call(f"mkdir run_{x}_{name_ext}", shell=True)

    # Get the same rule deck with gds output
    with open(rule_deck_path, 'r') as f:


is this generating a file? curious if you've considering using a templating system like https://jinja.palletsprojects.com/ or a multi-line format string? That could help convey a better understanding of the final shape of the file to the reader.

@proppy No it's generating an output folder structure for testing.

It seems to do a bit more than that: it reads rule_deck_path, then rewrite some of the line before appending them to marker_gen and then write the resulting strings to a file named markers.drc.

I'm curious why that rewriting is necessary (maybe add more comments), and if there is a better way (template?) to process this.

@proppy Yes, it generates a new rule deck from the original one and change the output to GDS layer rather than database output. For regression purposes.

can't this be a parameter passed to the rule deck using -rd rather than rewriting it on the fly?

@proppy No, that's a complicated thing to do. Also, the original rule deck doesn't need this output. It's only for regression purposes.

proppy · 2022-11-25T01:39:56Z

rules/klayout/drc/testing/run_regression.py

-    ly = 0
-    remove_if = False
+    ly         = 0
+    remove_if  = False

    # Get the small rule deck with gds output
    with open(rule_deck_path, 'r') as f:


curious why you need to rewrite the drc file?

I think when @mithro suggested having smaller file, he suggested having them maintained in the repo as separate files, not created on the fly by the test script.

@proppy Because, the regression needs to change the output to generate polygons to be able to check if the output in the right location or not. Yes, I know that @mithro has suggested to split the files. I already have done that and I could publish them. But the run time will explode as I told @mithro. The expected run time for DRC unit tests would be around 25 hours alone. While now using this current technique is about 30 mins or so.

the regression needs to change the output to generate polygons to be able to check if the output in the right location or not.

Are those check dynamic (i.e: they need to be rewritten everytime you run the test because they depends on the input) or can they be static? if the later maybe you could simply have the rewritten rules as part of the repo test suite rather than regenerating them?

@proppy No they can't be static. Because assume you added a new rule or removed a rule how would you know.

proppy · 2022-11-25T01:53:10Z

rules/klayout/drc/testing/run_switch_checking.py

-    os.system('mv -f {0}temp.log {0}{1}'.format(RUN_DIRECTORY,LOG_FILE_NAME))
+    check_call(
+        f'''
+mkdir -p {RUN_DIRECTORY}


couldn't you to this with the os module rather than doing it with commands?

@proppy will do.

proppy · 2022-11-25T01:53:23Z

rules/klayout/drc/testing/run_switch_checking.py

+
+    if sw_failed:
+        logging.error("## One of the test cases failed. Exit with failure:")
+        print(df)


@proppy Not sure what's the question?

sorry for not being clear, should you use the logging module rather than raw print statement?

@proppy will change.

proppy · 2022-11-25T01:53:29Z

rules/klayout/drc/testing/run_switch_checking.py

+    if sw_failed:
+        logging.error("## One of the test cases failed. Exit with failure:")
+        print(df)
+        exit(1)


Exception?

@proppy What's the difference? This will send a exit code of 1 which will fail github actions.

Yes, but an Exception give allow you to report more structured error and allow you to potentially handle it in the caller frames, i.e: imagine you have a test runner that run those test functions, you don't want the test itself to exit(1) (that would ends up the runner too as they are part of the same process) but you want to runner to be able to catch the Exception from the test and proceed accordingly.

@proppy But this is not a unit function. We only exit(1) after the entire run is complete.

proppy · 2022-11-25T01:53:34Z

rules/klayout/drc/testing/run_switch_checking.py

+    else:
+        logging.info("## All test cases passed.")
+        logging.info("## All Switch checking patterns:")
+        print(df)


@proppy what's the difference?

logging gives you more control over the leveling of the log and allow you to optionally direct the output to a file, see https://docs.python.org/3/library/logging.html

@proppy I got the point. Yes, that's why we use logging in the first place. The same comment as yours. But based on my experience printing Pandas DataFrame is nicer in print rather than logging. I could change it. But it won't be nice in the command line.

proppy · 2022-11-25T01:53:55Z

rules/klayout/drc/run_drc.py

@@ -188,37 +186,38 @@ def main():
            # Running DRC using klayout
            if (arguments["--antenna_only"]) and not (arguments["--density_only"]):
                logging.info(f"Running Global Foundries 180nm MCU antenna checks on design {name_clean} on cell {topcell_name}:")
-                os.system(f"klayout -b -r $PDK_ROOT/$PDK/gf180mcu_antenna.drc -rd input={path} -rd report={name_clean}_antenna_gf{arguments['--gf180mcu']}_gf{arguments['--gf180mcu']}.lyrdb -rd thr={thrCount} {switches}")
+                os.system(f"klayout -b -r {curr_path}/gf180mcu_antenna.drc -rd input={path} -rd report={name_clean}_antenna_gf{arguments['--gf180mcu']}_gf{arguments['--gf180mcu']}.lyrdb -rd thr={thrCount} {switches}")


subprocess here and below?

@proppy will do.

@proppy This entire script will be rewriten. I got the idea of your requests.

proppy · 2022-11-25T01:55:13Z

did an first overview of the python scripts as requested by @atorkmabrains (/cc @mithro)

mkkassem · 2022-11-25T18:24:56Z

@atorkmabrains

mkkassem · 2022-11-27T16:47:06Z

@atorkmabrains

atorkmabrains · 2022-11-28T16:43:00Z

@mkkassem I did answer all the questions and I'll make the necessary updates.

proppy · 2022-11-29T06:10:24Z

@atorkmabrains

@mkkassem I did answer all the questions

I still see some comments un-addressed, in https://github.com/google/globalfoundries-pdk-libs-gf180mcu_fd_pr/pull/78/files

and I'll make the necessary updates.

Please update this pull request w/ your changes, so that we can keep the review and the code changes in context.

atorkmabrains · 2022-11-29T06:22:09Z

@atorkmabrains I'll be working on it today and tomorrow.

Ngspice Regression

mkkassem changed the title ~~Updated exit code drc~~ Updated exit code for DRC regression Nov 24, 2022

mkkassem force-pushed the Updated-exit-code-drc branch from e52cca2 to ebf3385 Compare November 24, 2022 01:47

rules/klayout/drc: improve drc failure mode and documentation

f02b737

mkkassem force-pushed the Updated-exit-code-drc branch from ebf3385 to f02b737 Compare November 24, 2022 02:03

proppy requested changes Nov 25, 2022

View reviewed changes

proppy pushed a commit to proppy/globalfoundries-pdk-libs-gf180mcu_fd_pr that referenced this pull request Feb 6, 2023

Merge pull request google#78 from mabrains/ngspice_regression

c223ffe

Ngspice Regression

Updated exit code for DRC regression #78

Are you sure you want to change the base?

Updated exit code for DRC regression #78

Conversation

mkkassem commented Nov 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atorkmabrains Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proppy Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atorkmabrains Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proppy commented Nov 25, 2022

mkkassem commented Nov 25, 2022

mkkassem commented Nov 27, 2022

atorkmabrains commented Nov 28, 2022

proppy commented Nov 29, 2022

atorkmabrains commented Nov 29, 2022

atorkmabrains Nov 28, 2022 •

edited

Loading

proppy Nov 29, 2022 •

edited

Loading

atorkmabrains Nov 29, 2022 •

edited

Loading