Skip to content

(closes #3135) Fix cyclic dependencies by replacing Scalartype import-time instantiations - #3436

Merged
LonelyCat124 merged 11 commits into
masterfrom
scalartype_constructors
May 26, 2026
Merged

(closes #3135) Fix cyclic dependencies by replacing Scalartype import-time instantiations#3436
LonelyCat124 merged 11 commits into
masterfrom
scalartype_constructors

Conversation

@sergisiso

Copy link
Copy Markdown
Collaborator

I hit #3135 again where some cyclic dependencies cannot be prevented by adding the import inside methods because they happen when symbols are initialised at import-time. To remove this import time instantiations I opted for creating ScalarType staticmethods instead of the factory proposed in the issue. The change itself is small, but it is repeated many many times.

@codecov

codecov Bot commented May 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.96%. Comparing base (6662c41) to head (0c9fe9a).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3436   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files         391      391           
  Lines       54659    54683   +24     
=======================================
+ Hits        54640    54664   +24     
  Misses         19       19           

☔ View full report in Codecov by Sentry.
📢 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.

@sergisiso
sergisiso requested review from LonelyCat124 and arporter and removed request for arporter May 19, 2026 07:52
@sergisiso

Copy link
Copy Markdown
Collaborator Author

@LonelyCat124 @arporter This is ready for review

@sergisiso sergisiso changed the title (closes #3135) Prevent import time cyclic dependencies with Scalartype constructors (closes #3135) Fix cyclic dependencies by replacing Scalartype import-time instantiations May 19, 2026
@sergisiso sergisiso self-assigned this May 19, 2026

@LonelyCat124 LonelyCat124 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sergisiso I struggled to look through every single change (my browser is dying looking at the diffs...) but I found some minor issues to check with imports in examples and docs - can you check these? Then I can probably merge.

Comment thread doc/developer_guide/psyir.rst Outdated
Comment thread doc/user_guide/psyir.rst Outdated
@sergisiso

Copy link
Copy Markdown
Collaborator Author

@LonelyCat124 This is ready for another review.

@LonelyCat124

Copy link
Copy Markdown
Collaborator

Set ITs running as a sanity check but otherwise all is good.

@sergisiso

Copy link
Copy Markdown
Collaborator Author

@LonelyCat124 the integration test spotted a INTEGER_TYPE in lfric_apps. I will push a fix now

@sergisiso

sergisiso commented May 21, 2026

Copy link
Copy Markdown
Collaborator Author

@LonelyCat124 This is passing all integration test now, my lfric change from yesterday failed with signal 11, but after rerunning the action it has succeeded.

@sergisiso

Copy link
Copy Markdown
Collaborator Author

@LonelyCat124 This has finally passed all tests. I had to do it with multiple lfric_apps hash update attempts as I had no access to the system to test it directly.

@LonelyCat124

Copy link
Copy Markdown
Collaborator

Ok, now ITs pass I'll proceed to merge.

@LonelyCat124
LonelyCat124 merged commit 3b0e805 into master May 26, 2026
15 checks passed
@LonelyCat124
LonelyCat124 deleted the scalartype_constructors branch May 26, 2026 14:11
@haanahfrost haanahfrost mentioned this pull request Aug 3, 2026
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants