-
Notifications
You must be signed in to change notification settings - Fork 57
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 or-tools #2377
Add or-tools #2377
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff!
I can see some failed tests, https://beam-ci.tk/job/beam-4ci/7541/consoleFull:
|
test! |
016082c
to
93ae585
Compare
test! |
93ae585
to
4ebeb16
Compare
test! |
On Mac, i'm getting: this is probably due to protobuf-java, coming with or-tools. What about adding a gradle exception that excludes that jar from loading!? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this PR ready for review? I only see it loading and not doing anything with it right now. Also, is there a reason for using com.github? It's not a huge jump to figure out at this early stage that it is something in our control, but later it could pose more of a challenge for newcomers to understand that this is an lbnl tool - not GitHub.
This PR is for replace existing 3rd party library for OR-Tools, it is being used in AlonsoMora pooling algorithm. |
@wrashid @colinsheppard Any preference? Since it is for beam I'd say we stick with the beam namespace: |
test! |
3 similar comments
test! |
test! |
test! |
Actual exception somehow is not visible. Here is what I see when I run
|
@JustinPihony I think it fine that we leave the name as is. For example, beam-utilities has the following naming:
|
This change is
https://github.com/LBNL-UCB-STI/or-tools-wrapper