Skip to content

(Closes #2837) update python versions used in testing#3034

Merged
LonelyCat124 merged 8 commits into
masterfrom
2837_update_python
Jun 25, 2025
Merged

(Closes #2837) update python versions used in testing#3034
LonelyCat124 merged 8 commits into
masterfrom
2837_update_python

Conversation

@arporter

@arporter arporter commented Jun 23, 2025

Copy link
Copy Markdown
Member

Drops 3.7 and 3.8 from GHA. Just test on 3.10 and 3.13.

@arporter arporter self-assigned this Jun 23, 2025
@arporter arporter marked this pull request as draft June 23, 2025 15:47
@codecov

codecov Bot commented Jun 23, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.91%. Comparing base (bbb01ff) to head (4cb4d65).
Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3034      +/-   ##
==========================================
- Coverage   99.91%   99.91%   -0.01%     
==========================================
  Files         365      364       -1     
  Lines       52066    51697     -369     
==========================================
- Hits        52022    51652     -370     
- Misses         44       45       +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.

@arporter

Copy link
Copy Markdown
Member Author

Ready for review from @TeranIvy, @LonelyCat124, @hiker or @sergisiso. Updates the CI so that we only test with Python 3.10 and 3.13 (i.e. drops 3.7 and 3.8).

@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.

Hi @arporter

The changes here look fine.

I was searching through the code - there is a comment

    Note: This test failed with Python >3,<3.7 before explicit codecs
          were defined in the open(filename, ...) call.

in parse/utils_test.py that we can probably remove now as well.

In psyGen.py, there is a comment about "older versions" of python in a try/except w.r.t type checking of types like Union[int, bool]. This is now out-of-date, and the reason for this functionality is whe things are defined like Union[None, List[str]] (or even I think List[str] might be sufficient), we can't check the type still (we get a TypeError with the message Subscripted generics cannot be used with class and instance checks) - can you update this comment to reflect the continued need for this try/except (swapping to None | list[str] is also not enough to be able to do the type checking automatically. Its possible we can improve this behaviour by diving into the type further, but that is quite complex, or potentially require some other library - you can open an issue and add a TODO if you think this is something worth us spending time on, but I'm not sure1).

There is also discussion in a few places about more modern type hints (e.g. int | str instead of Union[int, str] and list[...] instead of List[...]. As we're moving to 3.10 this is now an option, so can you open an issue about changing to this - I think its a file-touch style improvement again, but at least the comments in src/psyclone/psyir/frontend/fparser2.py and src/psyclone/psyir/symbols/datatypes.py about pre-3.10 type hints could have a TODO regarding that as well.

@arporter

Copy link
Copy Markdown
Member Author

Thanks @LonelyCat124 for finding those other things. I've updated all of them and created #3035 as a reminder to people to fix other type hints as and when they touch them.
As for the type-checking, I think we can leave that as is for now. It feels like it could take a lot of effort.

@arporter

Copy link
Copy Markdown
Member Author

CI-happiness permitting, this is ready for another look now.

@LonelyCat124

Copy link
Copy Markdown
Collaborator

Happy with changes, going to run the integration tests to ensure everything is ok before merging.

@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.

@arporter looks good now, I'm happy with this and will merge it

@LonelyCat124 LonelyCat124 merged commit e8f0021 into master Jun 25, 2025
10 of 11 checks passed
@LonelyCat124 LonelyCat124 deleted the 2837_update_python branch June 25, 2025 14:42
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