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

Upgrade Operator version in Executor deps #2091

Closed
RafalSkolasinski opened this issue Jul 8, 2020 · 3 comments · Fixed by #2121
Closed

Upgrade Operator version in Executor deps #2091

RafalSkolasinski opened this issue Jul 8, 2020 · 3 comments · Fixed by #2121
Milestone

Comments

@RafalSkolasinski
Copy link
Contributor

RafalSkolasinski commented Jul 8, 2020

Sub Issue of #2088.

Current goal

Current goal is to update Operator version in Executor deps only.

Original comment below

As Executor reads SeldonDeployment CRD (provided currently via environmental variable) to be aware about the inference graph it needs to know about Operator's types like PredictorSpec and PredictiveUnit.

Currently Operator as a whole is a dependency of Executor -> which contributes a significant number of entries to go.sum.

Including only required types as Operator's subpackage (or just copying over and making sure these are in sync) will help to reduce number of dependencies in Executor significantly.

@RafalSkolasinski RafalSkolasinski added the triage Needs to be triaged and prioritised accordingly label Jul 8, 2020
@RafalSkolasinski
Copy link
Contributor Author

RafalSkolasinski commented Jul 8, 2020

BTW, this 2509235 commit in the cruft branch shows that just copying over Operator's types removes 273 lines of deps from go.sum.

@axsaucedo axsaucedo added this to the 1.3 milestone Jul 8, 2020
@ukclivecox ukclivecox added priority/p1 and removed triage Needs to be triaged and prioritised accordingly labels Jul 9, 2020
@RafalSkolasinski
Copy link
Contributor Author

It is currently being discussed if we want to fully remove the dependency (and how could that be achieved), move to depend only on subpackage of operator (seems reasonable if possible), or just continue depending on operator.

We should probably investigating pinning operator dependency more properly than v0.0.0-20200401123312-d4c435ea5217.

@RafalSkolasinski
Copy link
Contributor Author

Current angle is to just upgrade version of operator used by executor once operator's update is complete.

@RafalSkolasinski RafalSkolasinski changed the title Remove Executor's dependency on Operator Upgrade Operator version in Executor deps Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants