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

add clarabel wasm feature flag #63

Merged
merged 1 commit into from
Nov 2, 2024
Merged

add clarabel wasm feature flag #63

merged 1 commit into from
Nov 2, 2024

Conversation

Specy
Copy link
Contributor

@Specy Specy commented Oct 31, 2024

Adds the possibility to specify wasm feature flag to clarabel

Copy link
Collaborator

@lovasoa lovasoa left a comment

Choose a reason for hiding this comment

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

See this previous comment on the same topic: #58 (comment)

If you need this just for yourself, see: https://stackoverflow.com/questions/59994525/how-do-i-specify-features-for-a-sub-dependency-in-cargo

@Specy
Copy link
Contributor Author

Specy commented Nov 1, 2024

If you need this just for yourself, see: https://stackoverflow.com/questions/59994525/how-do-i-specify-features-for-a-sub-dependency-in-cargo

I've tried setting:

good_lp = { version = "1.8.1", features = ['clarabel'], default-features = false }
clarabel = { version = "*", features = ['wasm']} #also tried to put version as 0.9.0

but this does not seem to enable the feature flag.

Looking at the SO comment, it seems like this was disabled in rust 1.80?

What kind of test for CI were you thinking? simple build under wasm32-unknown-unknown?

@lovasoa
Copy link
Collaborator

lovasoa commented Nov 1, 2024

Ideally we would run wasm tests too

@Specy Specy requested a review from lovasoa November 2, 2024 14:38
@Specy
Copy link
Contributor Author

Specy commented Nov 2, 2024

i'm gonna also include minilp to the WASM tests

Copy link
Collaborator

@lovasoa lovasoa left a comment

Choose a reason for hiding this comment

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

This looks good !

Can we mention the new feature in the readme, maybe in the clarabel section ?

Then we can merge this !

@Specy
Copy link
Contributor Author

Specy commented Nov 2, 2024

I have absolutely no idea what that error is! Probably some mismatch in versions, I'll check later when I'm back home.

Should I add a new column in the table for things that support wasn't or add it as a new section note?

@lovasoa
Copy link
Collaborator

lovasoa commented Nov 2, 2024

I opened an issue on the clarabel side: oxfordcontrol/Clarabel.rs#134

It looks like the "wasm" feature should be removed. Clarabel could just compile for wasm natively if dependencies and conditional compilation were set up properly on their side: oxfordcontrol/Clarabel.rs#134

This does not prevent us to merge this pr on our side, to prevent future changes that would break wasm compatibility in good_lp itself.

@Specy
Copy link
Contributor Author

Specy commented Nov 2, 2024

It looks like the "wasm" feature should be removed. Clarabel could just compile for wasm natively if dependencies and conditional compilation were set up properly on their side: oxfordcontrol/Clarabel.rs#134

I'll try to make a PR there too to fix it

I'll also add wasm support as a new column to the table to show which solvers are able to be compiled to wasm. If in the future it will be able to compile to wasm-unknown-emscripten in wasm-pack I could also include soplex so this seems like a flexible solution. What do you think?

@Specy
Copy link
Contributor Author

Specy commented Nov 2, 2024

Ok the CI should be fixed now, just had to install node.

The clarabel repository seems a bit inactive so i'd say to merge this for now until the issue is fixed in clarabel

@lovasoa lovasoa merged commit 32687d6 into rust-or:main Nov 2, 2024
1 check passed
@lovasoa
Copy link
Collaborator

lovasoa commented Nov 2, 2024

Thank you for your contribution, @Specy !

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.

2 participants