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

Apps from Models #51

Merged
merged 49 commits into from
Dec 4, 2024
Merged

Apps from Models #51

merged 49 commits into from
Dec 4, 2024

Conversation

sebastian-quintero
Copy link
Member

@sebastian-quintero sebastian-quintero commented Nov 23, 2024

This PR introduces what I am calling the "Apps from Models" logic 👉🏻 we create a Nextmv Application from a Nextmv Python Model. This means that we can have a fully native experience in Python of creating a decision Model, pushing it to Cloud, and running it, without ever having to leave the Python session (like a script, or a notebook).

This is possible by leveraging mlflow and their "Models from Code" logic.

sebastian-quintero and others added 30 commits November 20, 2024 23:52
@sebastian-quintero sebastian-quintero changed the title App from Model Apps from Models Nov 24, 2024
nextmv/model.py Outdated Show resolved Hide resolved
Copy link
Member

@merschformann merschformann left a comment

Choose a reason for hiding this comment

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

I left a number of comments, let me know what you think.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Comment on lines +425 to +427
Place the following script in the root of your app directory and run it to
push your app to the Nextmv Cloud. This is equivalent to using the Nextmv
CLI and running `nextmv app push`.
Copy link
Member

Choose a reason for hiding this comment

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

I see that there is another option, but I am still missing a more CI/CD friendly way of deploying. I.e., we seem to be avoiding CLI here. If this is intentional, can we offer an entry point for nextmv-py that allows running the push without adding further scripts? Something like nextmvpy push --api-key <api-key> --app-id <app-id> maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, my intention for clarifying here is that people can rely on nextmv-py and not "know" about the CLI. I don’t know that the Python SDK should become a CLI tool as well, so I will defer that decision to others. In any case, that would be a follow up and I would address that in a different PR.

Copy link
Member

Choose a reason for hiding this comment

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

Fair point. I still think this is an odd or non-idiomatic experience for a non-notebook use-case (particularly for harmonic and CI/CD reasons). It would be nice to discuss this before we change the recommendation in general. 😊

nextmv/model.py Outdated Show resolved Hide resolved
nextmv/model.py Outdated Show resolved Hide resolved
nextmv/options.py Show resolved Hide resolved
nextmv/options.py Outdated Show resolved Hide resolved
nextmv/options.py Outdated Show resolved Hide resolved
Copy link
Member

@merschformann merschformann left a comment

Choose a reason for hiding this comment

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

I am still a bit concerned about some of the direction this takes us, but wouldn't block anything (particularly after our internal sync). 😊

@sebastian-quintero sebastian-quintero merged commit cfefabb into develop Dec 4, 2024
52 checks passed
@sebastian-quintero sebastian-quintero deleted the feature/plug-in-mlflow branch December 4, 2024 19:53
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.

4 participants