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

Vertex AI MLOps Accelerator #980

Closed

Conversation

paragsmhatre
Copy link

Generic accelerator for Machine Learning Operationalization using Vertex AI

@pull-request-size pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines. label Jan 26, 2023
Copy link
Member

@iht iht left a comment

Choose a reason for hiding this comment

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

There is a binary file in this PR (examples/vertex-ai-mlops-accelerator/PREDICT/model.joblib). What is that file for? Is that file generated somehow by this example?

Please use a linter with the Python code to make the code compliant with PEP8 (for instance, you can use https://pypi.org/project/pylint/). I have added comments where I have caught inconsistencies, but I may have missed it in other places.




stats_output = namedtuple('Outputs', ['worker_pool_specs'])
Copy link
Member

Choose a reason for hiding this comment

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

(following from previous comment) Then here you could just say:

    stats_output = WorkerPoolOutput(Outputs=['worker_pool_specs'])

examples/vertex-ai-mlops-accelerator/readme.md Outdated Show resolved Hide resolved
@paragsmhatre
Copy link
Author

@agold-rh @iht Sorry for the delay, got pulled into multiple other work items. We have updated the PR as per your suggestions. Please let me know if we need any updates.

@agold-rh agold-rh requested a review from iht March 28, 2023 13:58
@iht
Copy link
Member

iht commented Mar 29, 2023

Most of the comments I made have not been addressed. There are no changes to the Python code, the same issues remain. Please could you take a look at the comments?

@agold-rh
Copy link
Contributor

@paragsmhatre Could you take a look at @iht 's comment?

@paragsmhatre
Copy link
Author

@paragsmhatre Could you take a look at @iht 's comment?

Yes sure @iht @agold-rh , I have 1. applied Pylint , 2.removed binary files 3. Cleanup non-required files as per suggestions. I will revist the code and try to resolve all comments.

Thank you for the guidance.

Copy link
Contributor

@shakeebshams shakeebshams left a comment

Choose a reason for hiding this comment

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

Before merging, please ensure artifacts aren't outdated as VertexAI has changed significantly since this PR was opened

@agold-rh
Copy link
Contributor

Closed as stale. If I'm wrong, please re-open.

@agold-rh agold-rh closed this May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XXL Denotes a PR that changes 1000+ lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants