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.

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

package: Unpin topoly version #995

Merged
merged 3 commits into from
Nov 18, 2024
Merged

package: Unpin topoly version #995

merged 3 commits into from
Nov 18, 2024

Conversation

ns-rse
Copy link
Collaborator

@ns-rse ns-rse commented Nov 11, 2024

Wheels are now available for all OS's and architectures (thanks @prubach 👍 )

Closes #994

@ns-rse ns-rse added the packaging Issues pertaining to package structure, building and release. label Nov 11, 2024
@ns-rse
Copy link
Collaborator Author

ns-rse commented Nov 11, 2024

Rebase this branch after #988 has been merged to resolve the issues with pytest-regtest under OSX.

Wheels are now available for all OS's and architectures (thanks @prubach 👍 )

Closes #994
Tests using [`pytest-regtest-2.3.2`](https://pypi.org/project/pytest-regtest/#history) are [failing on
OSX](https://github.com/AFM-SPM/TopoStats/actions/runs/11780199328/job/32810287345?pr=988) pegging version to
`pytest-regtest==2.3.1` to see if this resolves the issue.
@ns-rse ns-rse force-pushed the ns-rse/994-unpin-topoly branch from faf8e67 to e115d0d Compare November 13, 2024 10:14
@ns-rse
Copy link
Collaborator Author

ns-rse commented Nov 13, 2024

I've manually updated the necessary files so that the change in nomenclature used by topoly to describe the connection types (? I don't even know what they are describing!) has changed from 4^2_1 > L4a1 and so forth.

Still feel we should be using pytest-regtest to handle these regression tests as its another thing to have to explain/teach how to do this manually for people who come in the future to work on the code base.

@MaxGamill-Sheffield
Copy link
Collaborator

Do we also need to pin numpy < 2 from Pawel's comment: "There seems to be some incompatibility with numpy 2.x. We built the 1.0.4 version before numpy 2.x was out and there are no issues with older numpy versions"

@ns-rse
Copy link
Collaborator Author

ns-rse commented Nov 13, 2024

Do we also need to pin numpy < 2 from Pawel's comment: "There seems to be some incompatibility with numpy 2.x. We built the 1.0.4 version before numpy 2.x was out and there are no issues with older numpy versions"

Full comment for context (bold emphasis is mine)...

BTW while I was investigating that matter I found another issue when using certain functions in Topoly (mostly GLN). There seems to be some incompatibility with numpy 2.x. We built the 1.0.4 version before numpy 2.x was out and there are no issues with older numpy versions but since I didn't limit the version, when you now create a new virtual environment numpy 2.x is by default installed on newer versions of python. I have no clue about the cause as it results in some segfaults but only with certain input data, even though we don't use numpy data types etc. in the compiled code. I'm considering restricting the numpy versions for now but need to investigate it further.

Until we get bitten by whatever the problem is (and we don't have enough details to know what that is) I'd say no, not at the moment1.

Numpy 2.0.0 was a significant release, hence the major semantic version change with breakages to be expected and was made 5 months ago. There have been minor revisions since (current stable version is 2.1.3 released 2024-11-02). Long term, keeping up-to-date with revisions is important otherwise you end up in the situation when I first came to work on TopoStats with Docker containers running unsupported software.

For that reason I'm not a fan of pinning version dependencies for long periods, it kicks the can of fixing whatever is broken further down the line.

Further, and perhaps more fundamentally, if topoly do decide to pin the Numpy version then I would expect pip to resolve that for us and install the correct version of Numpy and other packages so that things "Just Work(TM)".

Footnotes

  1. Perhaps there is a hint as maybe explicit data types are required in some instances.

@ns-rse ns-rse merged commit bffaf11 into main Nov 18, 2024
11 checks passed
@ns-rse ns-rse deleted the ns-rse/994-unpin-topoly branch November 18, 2024 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packaging Issues pertaining to package structure, building and release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unpin dependency on topoly==1.0.2
2 participants