(Closes #3461) duplicated script names by hiker · Pull Request #3468 · stfc/PSyclone

hiker · 2026-06-25T03:10:55Z

…s with the same name.

hiker · 2026-06-25T04:58:00Z

I've fixed the unit test failures by slightly changing the test (instead of setting sys.path, I chdir into the script directory).

We do get additional failure in example, since without modifying sys.path, helper scripts in the same directory as the psyclone scripts are NOT found anymore (unless you set PYTHONPATH=.). We could still modify sys.path in generator.py to accommodate that use case, I have started the CI to see if nemo/lfric relies on this.

…orting helper scripts, but clean up sys.pathin the end.

codecov · 2026-06-25T05:53:19Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (a74dfdf) to head (fdee332).

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #3468   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          397       397           
  Lines        55577     55569    -8     
=========================================
- Hits         55577     55569    -8

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

hiker · 2026-06-25T09:02:33Z

Failures in one example and nemo (lfric is fine) indicate that we should modify sys.path when importing and executing a script (so that helper scripts in the same directory as the script are found). That was done in ab86dd3, so once I've updated the documentation this should be ready to go.

…ipt_names

…ames do not need to be handled/removed anymore.

…ipt_names

hiker · 2026-07-28T04:55:02Z

IT passed, ready for review.

arporter

Thanks very much @hiker, this feels like a significant improvement. While reviewing it, I got very uncomfortable that our load_script routine (already) modifies global state and that this then has to be manually undone in multiple places after the script(s) have been used. I'm wondering whether there's a better way to do this that will prevent us making mistakes in the clean-up.

arporter · 2026-07-28T09:42:20Z

-@pytest.fixture(name="script_factory", scope="function")
-def create_script_factor(tmpdir):
-    ''' Fixture that creates a psyclone optimisation script given the string
+def script_factory(tmpdir, code: str) -> Path:


Since we're here, upgrade to tmp_path?

and throughout this file?

All done, I've replaced all of os.path, and using pathlib everywhere now.

Many thanks :-)

hiker · 2026-07-29T14:01:13Z

@arporter , I agree with the refactoring (including using a with). I was originally thinking of doing this, but that would have made this change (which changing the rather essential feature of importing the PSyclone script ;) ) much larger, and potentially more difficult to debug if something should break (e.g. in LFRic).

But if #1629 is being worked on, that would create huge conflicts, since this refactoring will need to introduce additional functions, i.e. moving code around quite a bit to avoid even deeper nesting.

I see three options:

I fix everything else you mentioned, and open a new ticket to refactor this properly. Once Use algorithm PSyIR when generating PSy-layer code #1629 is done, I'll fully implement this. Ideal if Use algorithm PSyIR when generating PSy-layer code #1629 is close to ready and has already refactored code in generator.py.
I think I can add a with statement without much refactoring, but nesting levels might become deep (and the code even harder to understand). This might have only a small impact on Use algorithm PSyIR when generating PSy-layer code #1629 - the deep nesting is mostly in the code_transformation_mode function, so this should not affect LFRic work at all.
I do it properly, and @sergisiso needs to sort it out 👿 Not ideal, that would be quite messy and I don't like it. But if not a lot of work in Use algorithm PSyIR when generating PSy-layer code #1629 has been done on generator.py (but elsewhere so far), that might(!) work.

Can you let me know what you prefer?

sergisiso · 2026-07-30T09:11:25Z

@hiker My PR won't be ready anytime soon, I found some blockers, but once completed it will simplify the generate.py.

Andy and me discussed this and we decided you can keep the current implementation with the "del" statements, but add a new issue/TODOs, as we want to come back to clean this up at some point.

…ipt_names

hiker · 2026-07-31T02:05:51Z

Ready for next review, IT triggered (issue #3514 for cleanup refactoring added).

…ipt_names

hiker · 2026-08-03T01:34:57Z

Documentation failure is caused by the NEMO wiki now requiring an account. IT triggered

arporter

Thanks Joerg. ITs were all green. Link failure (as you say) is not your fault and I've opened #3539 for that.
I just have one small comment on one of the test changes and then this is good to go.

arporter · 2026-08-04T13:51:11Z

-    config_name = os.path.join(os.path.dirname(os.path.abspath(__file__)),
-                               "test_files", "dummy_config.cfg")
-
+    config_path = Path(get_base_path("lfric")) / ".." / "dummy_config.cfg"


Path(get_base_path("lfric")) is just LFRIC_BASE_PATH?

arporter · 2026-08-04T13:53:33Z

-@pytest.fixture(name="script_factory", scope="function")
-def create_script_factor(tmpdir):
-    ''' Fixture that creates a psyclone optimisation script given the string
+def script_factory(tmpdir, code: str) -> Path:


Many thanks :-)

hiker added 2 commits June 25, 2026 12:25

#3461 Try to import scripts differently to allow for different script…

aa967b0

…s with the same name.

#3461 Fixed failing test that relied on setting sys.path.

22cbd78

hiker temporarily deployed to integration June 25, 2026 04:47 — with GitHub Actions Inactive

#3461 Add script dir back to sys.path to support psyclone scripts imp…

ab86dd3

…orting helper scripts, but clean up sys.pathin the end.

hiker temporarily deployed to integration June 25, 2026 05:44 — with GitHub Actions Inactive

hiker added 2 commits June 26, 2026 16:58

#3461 Updated documentation.

5112083

Merge remote-tracking branch 'origin/master' into 3461_duplicated_scr…

cd94747

…ipt_names

hiker temporarily deployed to integration June 26, 2026 07:13 — with GitHub Actions Inactive

hiker added 2 commits June 27, 2026 00:33

#3461 Replaced fixture with simple function since duplicated script n…

fb04cd2

…ames do not need to be handled/removed anymore.

#3461 Updated comments.

efe6459

LonelyCat124 changed the title ~~3461 duplicated script names~~ (Closes #3461) duplicated script names Jun 29, 2026

Merge remote-tracking branch 'origin/master' into 3461_duplicated_scr…

df18976

…ipt_names

hiker temporarily deployed to integration July 6, 2026 04:30 — with GitHub Actions Inactive

Merge remote-tracking branch 'origin/master' into 3461_duplicated_scr…

690fc9b

…ipt_names

hiker temporarily deployed to integration July 8, 2026 02:16 — with GitHub Actions Inactive

Merge remote-tracking branch 'origin/master' into 3461_duplicated_scr…

3207eeb

…ipt_names

hiker temporarily deployed to integration July 28, 2026 03:19 — with GitHub Actions Inactive

hiker added the ready for review label Jul 28, 2026

arporter added under review and removed ready for review labels Jul 28, 2026

arporter requested changes Jul 28, 2026

View reviewed changes

arporter added reviewed with actions and removed under review labels Jul 28, 2026

hiker mentioned this pull request Jul 31, 2026

Make generator.load_script more robust #3514

Open

hiker mentioned this pull request Jul 31, 2026

Import scripts with the same name more than once (and sys.path growing) #3461

Open

hiker added 4 commits July 31, 2026 11:48

#3461 Updated comments.

b78931d

#3461 Use tmp_path, and pathlib instead of os.

4c42bb8

#3461 Updated user guide.

f1f4624

Merge remote-tracking branch 'origin/master' into 3461_duplicated_scr…

4e517ed

…ipt_names

hiker temporarily deployed to integration July 31, 2026 02:02 — with GitHub Actions Inactive

hiker added ready for review and removed reviewed with actions labels Jul 31, 2026

Merge remote-tracking branch 'origin/master' into 3461_duplicated_scr…

fdee332

…ipt_names

hiker temporarily deployed to integration August 3, 2026 01:33 — with GitHub Actions Inactive

arporter added under review and removed ready for review labels Aug 4, 2026

arporter requested changes Aug 4, 2026

View reviewed changes

arporter added reviewed with actions and removed under review labels Aug 4, 2026

Uh oh!

Conversation

hiker commented Jun 25, 2026

Uh oh!

hiker commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hiker commented Jun 25, 2026

Uh oh!

hiker commented Jul 28, 2026

Uh oh!

arporter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arporter Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

arporter Jul 28, 2026

Choose a reason for hiding this comment

Uh oh!

hiker Jul 31, 2026

Choose a reason for hiding this comment

Uh oh!

arporter Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiker commented Jul 29, 2026

Uh oh!

sergisiso commented Jul 30, 2026

Uh oh!

hiker commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hiker commented Aug 3, 2026

Uh oh!

arporter left a comment

Choose a reason for hiding this comment

Uh oh!

arporter Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

arporter Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hiker commented Jun 25, 2026 •

edited

Loading

codecov Bot commented Jun 25, 2026 •

edited

Loading

hiker commented Jul 31, 2026 •

edited

Loading