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.

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

Update Customer Satisfaction to ZenML 0.20.2 #43

Merged
merged 3 commits into from Oct 26, 2022
Merged

Update Customer Satisfaction to ZenML 0.20.2 #43

merged 3 commits into from Oct 26, 2022

Conversation

safoinme
Copy link
Contributor

Describe changes

Update the custom satisfaction ZenFile to work with ZenML 0.20.2!

Pre-requisites

Please ensure you have done the following:

  • If my change requires a change to docs, I have updated the documentation accordingly.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Other (add details above)

@dagshub
Copy link

dagshub bot commented Oct 12, 2022

Join the discussion on DagsHub!

htahir1
htahir1 requested changes Oct 12, 2022
View reviewed changes
Copy link
Contributor

@htahir1 htahir1 left a comment

Choose a reason for hiding this comment

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

Nice one! Left a few comments

customer-satisfaction/README.md Outdated Show resolved Hide resolved
customer-satisfaction/materializer/custom_materializer.py Outdated
@@ -58,7 +58,7 @@ def handle_return(
self,
obj: Union[
str,
np.ndarray,
numpy.ndarray,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was facing an error with the materializer which didn't make sense! I was just modifying a lot of stuff and didn't revert this back! will do it right now

customer-satisfaction/run_pipeline.py
@@ -7,20 +7,17 @@
from zenml.integrations.mlflow.mlflow_utils import get_tracking_uri
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to run from a YAML file instead using the new method described here: https://docs.zenml.io/advanced-guide/pipelines/settings#method-3-configuring-with-yaml

@strickvl strickvl changed the title Update Custom Satisfaction to ZenML 0.20.2 Update Customer Satisfaction to ZenML 0.20.2 Oct 13, 2022
htahir1
htahir1 reviewed Oct 18, 2022
View reviewed changes
Copy link
Contributor

@htahir1 htahir1 left a comment

Choose a reason for hiding this comment

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

Looks good now but might make sense to add more details of running with the config file in the README?

strickvl
strickvl requested changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@strickvl strickvl left a comment

Choose a reason for hiding this comment

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

Some small changes in this review, but the biggest thing currently is that the deployment pipeline doesn't seem to work for me. Let's try again together tomorrow at some point?

customer-satisfaction/README.md Outdated Show resolved Hide resolved
customer-satisfaction/README.md Outdated Show resolved Hide resolved
strickvl
strickvl requested changes Oct 25, 2022
View reviewed changes
Copy link
Contributor

@strickvl strickvl left a comment

Choose a reason for hiding this comment

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

One other change I'd make is to increase the pinned zenml version to 0.20.5 (i.e. the current latest version) since we know it works.

Otherwise I've tested this on my end and it works, so nice job on making that work again!

@safoinme safoinme merged commit 2a206dc into main Oct 26, 2022
@strickvl strickvl deleted the fix/update-custom-satisfaction-zenml0.20 branch March 8, 2023 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htahir1 htahir1 htahir1 requested changes

@strickvl strickvl strickvl requested changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@safoinme @strickvl @htahir1