Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add tests for python 3.9 to 3.12 #124

Merged
merged 9 commits into from
May 15, 2024
Merged

Add tests for python 3.9 to 3.12 #124

merged 9 commits into from
May 15, 2024

Conversation

jorblancoa
Copy link
Collaborator

@jorblancoa jorblancoa commented Feb 14, 2024 •
edited
Loading

Context

Improve github actions CI by adding python versions 3.9 to 3.12.

Review

  • PR description is complete
  • Coding style (imports, function length, New functions, classes or files) are good
  • Unit/Scientific test added
  • Updated Readme, in-code, developer documentation

@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@jorblancoa jorblancoa changed the title StrEnum not working with older python versions Add tests for python 3.9 and 3.12 Feb 14, 2024
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@ferdonline ferdonline self-assigned this Apr 3, 2024
@BlueBrain BlueBrain deleted a comment from bbpbuildbot Apr 3, 2024
@bbpbuildbot

This comment has been minimized.

Sign in to view
@ferdonline
Copy link
Collaborator

Contrary to what was expected it seems that it is not due to the destructor.
I start to think that it's something specific to this system, namely the MPI4Py version.
@WeinaJi I see this is building Mpi4py from source. Do you think there's a way we can install a prebuilt one? It may save us from this problem

@WeinaJi
Copy link
Collaborator

WeinaJi commented Apr 10, 2024

@ferdonline , it seems that we can use pip install mpi4py while rebuilding h5py. I tested with the docker image. Can you change that in the GH actions script?

@WeinaJi
Copy link
Collaborator

WeinaJi commented Apr 10, 2024 •
edited
Loading

I've pushed a change to use prebuilt mpi4py but the error is still there.

@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
WeinaJi
WeinaJi reviewed Apr 23, 2024
View reviewed changes
neurodamus/node.py Outdated Show resolved Hide resolved
matz-e
matz-e previously approved these changes Apr 25, 2024
View reviewed changes
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@ferdonline ferdonline marked this pull request as ready for review April 30, 2024 08:58
ferdonline
ferdonline previously approved these changes Apr 30, 2024
View reviewed changes
Copy link
Collaborator

@ferdonline ferdonline left a comment

Choose a reason for hiding this comment

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

Looks good to me

@jorblancoa @WeinaJi How do you feel about the changes I did trying to simplify the destructor? We now know it wasn't the problem in this case. Shall we keep it anyway?

@jorblancoa
Copy link
Collaborator Author

Looks good to me

@jorblancoa @WeinaJi How do you feel about the changes I did trying to simplify the destructor? We now know it wasn't the problem in this case. Shall we keep it anyway?

For me the changes seem reasonable!

@jorblancoa jorblancoa requested review from WeinaJi and matz-e May 13, 2024 18:59
WeinaJi
WeinaJi reviewed May 14, 2024
View reviewed changes
Copy link
Collaborator

@WeinaJi WeinaJi left a comment

Choose a reason for hiding this comment

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

More caching will come from #166. This PR can be merged first.

@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@bbpbuildbot

This comment has been minimized.

Sign in to view
@WeinaJi WeinaJi changed the title Add tests for python 3.9 and 3.12 Add tests for python 3.9 to 3.12 May 14, 2024
@bbpbuildbot

This comment has been minimized.

Sign in to view
@WeinaJi
Copy link
Collaborator

WeinaJi commented May 14, 2024

@matz-e @ferdonline , caching and multiple python versions are in place. Shall we merge it?

@bbpbuildbot
Copy link

Logfiles from GitLab pipeline #211134 (:white_check_mark:) have been uploaded here!

Status and direct links:

@WeinaJi WeinaJi merged commit dd59eef into main May 15, 2024
9 checks passed
@WeinaJi WeinaJi deleted the jblanco/revert_str_enum branch May 15, 2024 11:10
atemerev pushed a commit that referenced this pull request May 24, 2024
@jorblancoa @WeinaJi @atemerev
Add tests for python 3.9 to 3.12 (#124)
3bfba2f 
## Context
Improve github actions CI by adding python versions 3.9 to 3.12.

## Review
* [x] PR description is complete
* [x] Coding style (imports, function length, New functions, classes or
files) are good
* [ ] Unit/Scientific test added
* [ ] Updated Readme, in-code, developer documentation

---------

Co-authored-by: Weina Ji <[email protected]>
WeinaJi added a commit that referenced this pull request Oct 14, 2024
@jorblancoa @WeinaJi
Add tests for python 3.9 to 3.12 (#124)
2d976d6 
## Context
Improve github actions CI by adding python versions 3.9 to 3.12.

## Review
* [x] PR description is complete
* [x] Coding style (imports, function length, New functions, classes or
files) are good
* [ ] Unit/Scientific test added
* [ ] Updated Readme, in-code, developer documentation

---------

Co-authored-by: Weina Ji <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WeinaJi WeinaJi WeinaJi approved these changes

@ferdonline ferdonline Awaiting requested review from ferdonline

@matz-e matz-e Awaiting requested review from matz-e

Assignees

@ferdonline ferdonline

@jorblancoa jorblancoa

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jorblancoa @bbpbuildbot @ferdonline @WeinaJi @matz-e