Skip to content

Bring back python 3.9 in the integration tests#3043

Merged
arporter merged 6 commits into
masterfrom
bring_back_3_9
Jul 17, 2025
Merged

Bring back python 3.9 in the integration tests#3043
arporter merged 6 commits into
masterfrom
bring_back_3_9

Conversation

@sergisiso

@sergisiso sergisiso commented Jul 2, 2025

Copy link
Copy Markdown
Collaborator

python3.9 is not EOLd yet and Archer2 which has LFRic users defaults to it. So it would be good to test against it.

@sergisiso sergisiso force-pushed the bring_back_3_9 branch 4 times, most recently from 463dc25 to 4824b32 Compare July 2, 2025 11:32
@codecov

codecov Bot commented Jul 2, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.92%. Comparing base (af5cb94) to head (bc07a25).
Report is 7 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3043   +/-   ##
=======================================
  Coverage   99.92%   99.92%           
=======================================
  Files         366      366           
  Lines       51721    51721           
=======================================
+ Hits        51680    51681    +1     
+ Misses         41       40    -1     

☔ 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

Copy link
Copy Markdown
Collaborator Author

@LonelyCat124 This is ready for review. I am bringing back python 3.9 as EPCC and other people from NG-Arch are using it on Archer2. I am tempted to suggest that we update the previous changelog instead of creating a new issue for this as it was only a couple of PRs ago and the issue didn't specify 3.9.

@LonelyCat124

Copy link
Copy Markdown
Collaborator

Hi @sergisiso - I'll review this when I'm back from leave next week - can you set the integration running in the mean time?

@schreiberx

Copy link
Copy Markdown
Collaborator

@sergisiso Thanks. I had some issues with Python 3.9 with the most recent Psyclone version.

@sergisiso sergisiso requested a review from arporter July 17, 2025 07:58
@sergisiso

Copy link
Copy Markdown
Collaborator Author

@arporter @LonelyCat124 Can you give some priority to this. It is very easy to introduce breaking changes if its not tested.

@arporter

Copy link
Copy Markdown
Member

I'll take it now.

@arporter arporter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All looks fine @sergisiso, thanks for fixing this (and apologies for being too hasty in removing it). Just a request to make a new issue to drop 3.9 when it goes EOL (in October 2025) so that we can put back the commented-out test.

Comment thread src/psyclone/tests/psyir/transformations/transformations_test.py Outdated
Comment thread src/psyclone/psyir/symbols/datatypes.py
Comment thread src/psyclone/psyir/frontend/fparser2.py
@sergisiso

Copy link
Copy Markdown
Collaborator Author

@arporter I don't think I agree with the comments, we don't need a TODO for things that will continue to be valid, there are many Optional, Unions through the codebase and I don't think it is a goal to remove those. The commented out assert may be a better location but even then, I don't think we should deprecate 3.9 anytime soon (October), since it is the default on Archer2 and probably means it is the default in similar RedHat/Cray machines.

@arporter

Copy link
Copy Markdown
Member

The commented out assert may be a better location but even then, I don't think we should deprecate 3.9 anytime soon (October), since it is the default on Archer2 and probably means it is the default in similar RedHat/Cray machines.

Good point - we'll probably need 3.9 for longer as you say. In that case, please could you change the test back to something that works for all versions so that we don't have commented-out code.

@LonelyCat124

Copy link
Copy Markdown
Collaborator

@arporter I don't think I agree with the comments, we don't need a TODO for things that will continue to be valid, there are many Optional, Unions through the codebase and I don't think it is a goal to remove those. The commented out assert may be a better location but even then, I don't think we should deprecate 3.9 anytime soon (October), since it is the default on Archer2 and probably means it is the default in similar RedHat/Cray machines.

I think we should have a rule with how we want to do Union or Optional types (at least for in-code type hints) and have the code be consistent, my preference would be the newer a | b or a | None style, but I'm not too bothered either way. The latter requires 3.10 though I think so we can't move to it until then.

@sergisiso

Copy link
Copy Markdown
Collaborator Author

@arporter Ready for another review

@arporter

Copy link
Copy Markdown
Member

I think we should have a rule with how we want to do Union or Optional types (at least for in-code type hints) and have the code be consistent, my preference would be the newer a | b or a | None style, but I'm not too bothered either way. The latter requires 3.10 though I think so we can't move to it until then.

Me too but yes, it will have to wait. I always forget how slowly HPC platforms evolve.

@arporter arporter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks Sergi. This can be merged now.

@arporter arporter merged commit e5dd46b into master Jul 17, 2025
11 checks passed
@arporter arporter deleted the bring_back_3_9 branch July 17, 2025 10:13
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.

4 participants