8000 Mac support for or_tools by zivnevo · Pull Request #1840 · fybrik/fybrik · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Mac support for or_tools #1840

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
Merged
merged 13 commits into from
Jan 3, 2023
Merged

Mac support for or_tools #1840

merged 13 commits into from
Jan 3, 2023

Conversation

zivnevo
Copy link
Collaborator
@zivnevo zivnevo commented Dec 5, 2022 •
edited
Loading

Also upgrading to OR Tools version 9.5

Should solve some of the issues in #1525

@zivnevo
Mac support for or_tools
4eacd0b 
Also upgrading to 9.5

Signed-off-by: Ziv Nevo <nevo@il.ibm.com>
@zivnevo
Properly print error
3d67653 
Signed-off-by: Ziv Nevo <nevo@il.ibm.com>
@zivnevo zivnevo mentioned this pull request Dec 5, 2022
Closed
@zivnevo zivnevo requested a review from shlomitk1 January 3, 2023 06:56
@zivnevo zivnevo requested a review from roytman January 3, 2023 06:57
shlomitk1
shlomitk1 reviewed Jan 3, 2023
View reviewed changes
Copy link
Contributor
@shlomitk1 shlomitk1 left a comment

Choose a reason for hiding this comment

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

Currently, the optimizer image is hard-coded to image: "ghcr.io/fybrik/optimizer:0.0.1" which was built quite a while ago. In order to proper test the Dockerfile, the following is required:

  • replace solver.image with "optimizer" for all charts/fybrik/values*.yaml
  • add docker-build for optimizer in Makefile (where manager is built)
  • add docker-push for optimizer in the git push action

@zivnevo
Copy link
Collaborator Author
zivnevo commented Jan 3, 2023

Currently, the optimizer image is hard-coded to image: "ghcr.io/fybrik/optimizer:0.0.1" which was built quite a while ago. In order to proper test the Dockerfile, the following is required:

* replace solver.image with "optimizer" for all charts/fybrik/values*.yaml

* add docker-build for optimizer in Makefile (where manager is built)

* add docker-push for optimizer in the git push action

I'm not sure I understand which tests take which image from which location. In practice, Tekton Tests passed only after I modified the Dockerfile to run the solver install script.
How about a new issue for the changes you proposed?

@shlomitk1
Copy link
Contributor

I'm not sure I understand which tests take which image from which location. In practice, Tekton Tests passed only after I modified the Dockerfile to run the solver install script. How about a new issue for the changes you proposed?

Tekton tests do not use the solver image, they use the local installation instead. I believe that they pass because you changed the install script. The changes to Dockerfile do not have any effect at all because "make docker-build docker-push" is never called for the optimizer.
My concern is that your changes to Dockerfile have not been tested because of the hard-coded old image that was created in March. If you remove Dockerfile and Makefile from this PR, it's OK to have it merged.

@shlomitk1 shlomitk1 merged commit 71dcd28 into fybrik:master Jan 3, 2023
@zivnevo zivnevo 8000 deleted the or_tools_mac_support branch January 3, 2023 12:18
aradhalevy pushed a commit to aradhalevy/fybrik that referenced this pull request Jan 10, 2023
@zivnevo @shlomitk1
Mac support for or_tools (fybrik#1840)
13aa9e4 
* Mac support for or_tools

Also upgrading to 9.5
Signed-off-by: Ziv Nevo <nevo@il.ibm.com>
Co-authored-by: Shlomit Koyfman <shlomitk@il.ibm.com>
@revit13
Copy link
Collaborator
revit13 commented Jan 15, 2023 •
edited by fybrik-bot bot
Loading

/cherry-pick branch=releases/1.2.1

Failed - see https://github.com/fybrik/fybrik/actions/runs/3923271330

revit13 pushed a commit to revit13/fybrik that referenced this pull request Jan 15, 2023
@zivnevo @shlomitk1 @revit13
Mac support for or_tools (fybrik#1840)
eb5c1a4 
* Mac support for or_tools

Also upgrading to 9.5
Signed-off-by: Ziv Nevo <nevo@il.ibm.com>
Co-authored-by: Shlomit Koyfman <shlomitk@il.ibm.com>
revit13 added a commit that referenced this pull request Jan 15, 2023
@revit13 @shlomitk1
Mac support for or_tools (#1840) (#1888)
2318746 
* Mac support for or_tools

Also upgrading to 9.5
Signed-off-by: Ziv Nevo <nevo@il.ibm.com>
Co-authored-by: Shlomit Koyfman <shlomitk@il.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shlomitk1 shlomitk1 shlomitk1 approved these changes

@roytman roytman Awaiting requested review from roytman

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zivnevo @shlomitk1 @revit13
0