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

Depricate persistence #3104

Merged
merged 10 commits into from
Apr 22, 2021
Merged

Conversation

nadinet
Copy link
Contributor

@nadinet nadinet commented Apr 1, 2021

What this PR does / why we need it:
Remove "PERSISTENCE" Redis functionality and documentation.
"soft" deprication, flag is ignored.
Which issue(s) this PR fixes:

Fixes #2888

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@nadinet nadinet requested a review from adriangonz April 1, 2021 19:51
@seldondev
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign axsaucedo
You can assign the PR to them by writing /assign @axsaucedo in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@adriangonz
Copy link
Contributor

Nice one @nadinet! We may also need to re-generate the licenses information to remove all the redis licensing info.

@adriangonz
Copy link
Contributor

/test integration

@adriangonz
Copy link
Contributor

/test notebooks

@adriangonz
Copy link
Contributor

@nadinet should we also remove the --persistence reference from the s2i/bin/run script in the S2I Python wrapper template?

exec seldon-core-microservice $MODEL_NAME --service-type $SERVICE_TYPE --persistence $PERSISTENCE

I just realised that the same script also forces for the PERSISTENCE flag to be present, so maybe that's one we can also remove:

#check environment vars
if [[ -z "$MODEL_NAME" || -z "$SERVICE_TYPE" || -z "$PERSISTENCE" ]]; then
echo "Failed to find required env vars MODEL_NAME, SERVICE_TYPE, PERSISTENCE"
exit 1
else

@adriangonz
Copy link
Contributor

/test integration

@adriangonz
Copy link
Contributor

/test notebooks

@seldondev seldondev force-pushed the remove_redis_persistence branch from f16e97e to f50b1a8 Compare April 7, 2021 17:39
@axsaucedo
Copy link
Contributor

/test integration

@axsaucedo
Copy link
Contributor

/test notebooks

@@ -44,6 +44,9 @@ else
fi

echo "starting microservice"
exec seldon-core-microservice $MODEL_NAME --service-type $SERVICE_TYPE --persistence $PERSISTENCE

if[-n "$PERSISTENCE"]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect syntax

@axsaucedo
Copy link
Contributor

/test integration

@axsaucedo
Copy link
Contributor

/test notebooks

@axsaucedo
Copy link
Contributor

axsaucedo commented Apr 8, 2021

It seems the tests have found a couple of issues:

On the integration tests it failed because mlflow server seems to re-install the older version of seldon core

image

image

Basically showing error importing redis

image

Fixed by removing extra installation, and uses the base dependency correctly

image

On the notebooks tests it's one test failed

image

The failed test also has an mlfow server run at the bottom, ran the rest of the examples successfully

image

@axsaucedo
Copy link
Contributor

/test integration

@axsaucedo
Copy link
Contributor

/test notebooks

@RafalSkolasinski
Copy link
Contributor

/test integration

@axsaucedo
Copy link
Contributor

/test notebooks

@axsaucedo
Copy link
Contributor

/test integration

@axsaucedo
Copy link
Contributor

/test notebooks

@axsaucedo
Copy link
Contributor

/test integration

5 similar comments
@axsaucedo
Copy link
Contributor

/test integration

@axsaucedo
Copy link
Contributor

/test integration

@axsaucedo
Copy link
Contributor

/test integration

@adriangonz
Copy link
Contributor

/test integration

@RafalSkolasinski
Copy link
Contributor

/test integration

@RafalSkolasinski
Copy link
Contributor

Just to check if changes in original mlflow bucket managed to propagate
/test notebooks

@axsaucedo axsaucedo merged commit 2a09a8d into SeldonIO:master Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove "PERSISTENCE" Redis functionality and documentation
5 participants