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

FIX snapML error #11

Open
wants to merge 43 commits into
base: main
Choose a base branch
from

Conversation

wassimmazouz
Copy link
Contributor

This PR aims to fix the incompatibility between current snapml and numpy versions.

@wassimmazouz
Copy link
Contributor Author

@mathurinm original error solved. The remaining error in MacOS might be because libomp isn't installed by default and it's needed for snapml. Maybe adding a brew install libomp might solve it but it requires the test_benchmarks.yml file to be changed.

@mathurinm
Copy link
Contributor

The issue is that snapml released wheels built wtih numpy 1.*, that are not compatible with numpy 2. They do not release source (only wheels), so we cannot build the wheels ourselves.

It's very constraining to pin versions exactly. Can you require "numpy<2" instead ? And add a comment above this line to explain why it's needed, so that it can be removed when adequate wheels are released (with a TODO)

For libomp, go ahead !

solvers/snapml.py Outdated Show resolved Hide resolved
@@ -16,7 +16,7 @@ on:

jobs:
benchopt_dev:
uses: benchopt/template_benchmark/.github/workflows/test_benchmarks.yml@main
uses: wassimmazouz/template_benchmark/.github/workflows/test_benchmarks.yml@adding-libomp-install
Copy link
Contributor

Choose a reason for hiding this comment

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

clever !

@wassimmazouz
Copy link
Contributor Author

@mathurinm CI all 🟢. I've opened a PR on the template benchmark repository to incorporate these changes.

@wassimmazouz wassimmazouz requested a review from mathurinm July 18, 2024 09:31
@@ -0,0 +1,58 @@
============================
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't test it, so I assume it worked on a machine you used @wassimmazouz ?

@tomMoral wdyt, is this overkill ? if the same resource is accessible online maybe we can just link to it in the snapml solver ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mathurinm These were the minimal changes that helped pass the tests. However, I haven't tried this on any machine.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I would just add a link to the tutorial in the README.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants