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

specs: VEP-1739 Support for Multiple Python Versions #1748

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

doks5
Copy link
Contributor

@doks5 doks5 commented Mar 14, 2023

This change introduces VDP-1739, which aims at proposing an improvement to Versatile Data Kit by adding support for using different python versions for data job deployments.

Testing Done: N/A

@doks5 doks5 linked an issue Mar 14, 2023 that may be closed by this pull request
Copy link
Collaborator

@antoniivanov antoniivanov 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 suggesting and driving this change. The proposal is generally well-structured and addresses the problem at hand effectively.

I have made some suggestions to further improve the proposal.

@doks5 doks5 force-pushed the person/andonova/vep-multi-python-versions branch from 3bb4163 to f37f23d Compare March 15, 2023 09:01
@doks5 doks5 changed the title [DRAFT] specs: Introduce VEP-1739 specs: Introduce VEP-1739 Mar 15, 2023
@doks5 doks5 changed the title specs: Introduce VEP-1739 [DRAFT] specs: Introduce VEP-1739 Mar 15, 2023
Copy link
Contributor

@ivakoleva ivakoleva left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for driving this effort for improving usability in terms of granular data job runtime environment configurations.

specs/vep-1739-multiple-python-versions/README.md Outdated Show resolved Hide resolved
specs/vep-1739-multiple-python-versions/README.md Outdated Show resolved Hide resolved
@doks5 doks5 force-pushed the person/andonova/vep-multi-python-versions branch from f37f23d to da75797 Compare March 15, 2023 22:03
Copy link
Collaborator

@antoniivanov antoniivanov 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 to me. I made suggestion to improve sequence diagram only.

@doks5 doks5 force-pushed the person/andonova/vep-multi-python-versions branch from da75797 to 3e6bf0f Compare March 16, 2023 11:13
@antoniivanov
Copy link
Collaborator

I approved it because I think the Motivation, Requirements and High level design sections are good to be merged.

For the detailed design I think we should cover

  • Troubleshooting - what are the failure modes and what we can do to resolve them, what users can to do
  • Test Plan
  • Upgrade / Downgrade Strategy
  • Database data model changes (we need to keep python version somewhere).

It's fine if some details are not yet clear and can be filled in the middle of the implementation when they become clearer

@doks5 doks5 force-pushed the person/andonova/vep-multi-python-versions branch from 3e6bf0f to e69a6a7 Compare March 16, 2023 15:57
@doks5 doks5 changed the title [DRAFT] specs: Introduce VEP-1739 specs: Introduce VEP-1739 Mar 16, 2023
@doks5 doks5 marked this pull request as ready for review March 16, 2023 15:59
@doks5
Copy link
Contributor Author

doks5 commented Mar 16, 2023

I approved it because I think the Motivation, Requirements and High level design sections are good to be merged.

For the detailed design I think we should cover

* Troubleshooting - what are the failure modes and what we can do to resolve them, what users can to do

* Test Plan

* Upgrade / Downgrade Strategy

* Database data model changes (we need to keep python version somewhere).

It's fine if some details are not yet clear and can be filled in the middle of the implementation when they become clearer

Hi, @tozka

Indeed, more details will be added with subsequent PR. For this one, I've updated the diagrams and added the initial parts of the detailed design and the public API changes.

@doks5 doks5 requested a review from mivanov1988 March 16, 2023 16:01
@antoniivanov antoniivanov changed the title specs: Introduce VEP-1739 specs: VEP-1739 Support for Multiple Python Versions Mar 16, 2023
@mivanov1988
Copy link
Contributor

Few comments regarding the sequence diagram:

  • The CS first starts the deployment and then returns the status to the client that the deployment is started.
  • What actually do the following operations - "Request data job base image with specified python version" and "Request data vdk image with specified python version"?
  • Registry -> Docker Registry

This change introduces VDP-1739, which aims at proposing an improvement to Versatile
Data Kit by adding support for using different python versions for data job
deployments.

Testing Done: N/A

Signed-off-by: Andon Andonov <andonova@vmware.com>
@doks5 doks5 force-pushed the person/andonova/vep-multi-python-versions branch from e69a6a7 to 3e5a10a Compare March 17, 2023 14:33
@doks5
Copy link
Contributor Author

doks5 commented Mar 17, 2023

Few comments regarding the sequence diagram:

* The CS first starts the deployment and then returns the status to the client that the deployment is started.

* What actually do the following operations - "Request data job base image with specified python version"  and  "Request data vdk image with specified python version"?

* Registry -> Docker Registry

Hi, @mivanov1988

Just updated the diagram to make the actual flow clearer.

@doks5 doks5 merged commit 71e2de7 into main Mar 17, 2023
@doks5 doks5 deleted the person/andonova/vep-multi-python-versions branch March 17, 2023 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Multiple Python Versions in VDK
5 participants