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

MLFlowModel rewrite #180

Merged
merged 2 commits into from
Oct 27, 2023
Merged

Conversation

SilvestriStefano
Copy link
Contributor

@SilvestriStefano SilvestriStefano commented Sep 27, 2023

Hello,
this is a fix for issue #179
use yaml to read MLFlowModel file as suggested by user @pulungw

I ran the test test_mlflow_model.py and it passed.

I also ran the code in the notebook pulungw/sascode/python/mlflow_sample.ipynb and it worked fine.
image

Signed-off-by: Stefano Silvestri [email protected]

@smlindauer smlindauer self-requested a review October 25, 2023 16:26
Copy link
Collaborator

@smlindauer smlindauer left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @SilvestriStefano! It looks like the yaml package is not a built-in Python package. Would we have to include an additional dependency in the setup.py file for this to work for other users?

@smlindauer smlindauer self-assigned this Oct 25, 2023
@SilvestriStefano
Copy link
Contributor Author

SilvestriStefano commented Oct 26, 2023

Hi,
sasctl already requires the pyyaml package :)

install_requires=["pandas>=0.24.0", "requests", "pyyaml", "packaging"],

which is then called for example in core.py :
import yaml

@smlindauer
Copy link
Collaborator

Good to know that I can sometimes read 😜.

If the reqs are already in place, then I am going forward with merging this.

@smlindauer smlindauer merged commit 44e4786 into sassoftware:master Oct 27, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants