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

Introduce ps-stacks prescription for ps-cv-pytorch #17213

Merged

Conversation

pacospace
Copy link
Contributor

Signed-off-by: Francesco Murdaca [email protected]

What type of PR is this?

/kind feature

Related issues or additional information of the supplied change

See: https://chat.google.com/room/AAAAVjnVXFk/YwV3qk8eoxA (thanks @fridex for guidance)

Description

Introduce new folder for ps-stacks prescriptions. See: https://github.com/thoth-station/ps-cv/blob/master/overlays/ps-cv-pytorch/Pipfile

Related-To: AICoE/recommend-base-image-tutorial#1

Signed-off-by: Francesco Murdaca <[email protected]>
@sesheta sesheta added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 16, 2021
@sesheta sesheta requested review from fridex and goern September 16, 2021 09:57
@sesheta sesheta added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 16, 2021
@pacospace pacospace requested a review from fridex September 17, 2021 06:59
@pacospace
Copy link
Contributor Author

@fridex PTAL when you have moment
/assign @fridex

@pacospace pacospace force-pushed the introduce-ps-stack-prescription branch from e31c649 to 8f7e5d3 Compare September 21, 2021 10:28
@sesheta
Copy link
Member

sesheta commented Sep 21, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from fridex after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@pacospace pacospace requested a review from fridex September 21, 2021 10:28
run:
justification:
- type: INFO
message: Consider using predictive stack for computer vision with Pytorch
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the match part, this message will be shown also when tensorboard, jupyter-tensorboard or pillow will be resolved. Is it expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe not, maybe only when any of those are combined with the main packages, Pytorch and opencv. I will remove them then. Thanks @fridex !

Copy link
Contributor

Choose a reason for hiding this comment

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

What about adjusting it to make it more generic - not specific to Pytorch, but to a computer vision stack?

Copy link
Contributor Author

@pacospace pacospace Sep 21, 2021

Choose a reason for hiding this comment

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

Maybe not, maybe only when any of those are combined with the main packages, Pytorch and opencv. I will remove them then. Thanks @fridex !

Actually thinking a little bit more on this, if their software stacks is a subset of the list of packages stated in the prescription, we could recommend a base image (or more in the future maybe, sorted by?), so it might make sense to keep also the others because if they have have more packages that those ones in the prescription (direct one) we won't recommend it right?, wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

The packages can be kept in the prescription as the recommender system can provide suggestions on which base image to use. The justification message can be adjusted to be more generic, not specific to Pytorch. Users can use the container image if it provides used packages.

if they have have more packages that those ones in the prescription (direct one) we won't recommend it right?, wdyt?

We can still advertise the base images and users can build on top of them. If the intention is to ecommend the base image based on the direct dependencies, you can provide boot prescription unit that can act on direct dependencies used in the application - see https://thoth-station.ninja/docs/developers/adviser/prescription/boots.html#boot-match

Copy link
Contributor Author

@pacospace pacospace Sep 21, 2021

Choose a reason for hiding this comment

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

The packages can be kept in the prescription as the recommender system can provide suggestions on which base image to use. The justification message can be adjusted to be more generic, not specific to Pytorch. Users can use the container image if it provides used packages.
Well this prescription is for Computer Vision with Pytorch image:

      justification:
      - type: INFO
        message: Consider using predictive stack for computer vision with Pytorch
        link: https://quay.io/repository/thoth-station/ps-cv-pytorch

if they have have more packages that those ones in the prescription (direct one) we won't recommend it right?, wdyt?

We can still advertise the base images and users can build on top of them. If the intention is to ecommend the base image based on the direct dependencies, you can provide boot prescription unit that can act on direct dependencies used in the application - see https://thoth-station.ninja/docs/developers/adviser/prescription/boots.html#boot-match

Is this a different prescription to be prepared? Isn't the type: wrap doing that purpose?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a different prescription to be prepared? Isn't the type: wrap doing that purpose?

The same way as Kubernetes provides different objects, such as Job or Deployment, the same way the resolver provides types of pipeline units as an abstraction to configure the resolution process. wrap is executed at the end of the resolution process on a fully resolved set of packages whereas boot is executed at the beginning of the resolution process on direct dependencies (just for completeness: there are other inputs that they can act on, but that is not relevant for this case).

Now, the difference in wrap.match part - this configuration will add the justification mentioned when jupyter-tensorboard OR torch is present:

    match:
    - state:
        resolved_dependencies:
        - name: jupyter-tensorboard
     - state:
        resolved_dependencies:
        - name: torch

Whereas this configuration will add the justification when jupyter-tensorboard AND torch are present in the resolved set of packages at the same time:

    match:
    - state:
        resolved_dependencies:
        - name: jupyter-tensorboard
        - name: torch

Additional version ranges and index configuration can be applied.

To recommend the base image, it might be good to pick "representatives" that describe the container image the best. This way, we can recommend a container image for computer vision with pytorch if pytorch is present. On the other hand, jupyter-tensorboard might not be a good package to justify recommending pytorch specific computer vision container images.

This might be a good start, then we can perform a more complicated things as discussed in AICoE/recommend-base-image-tutorial#1

Copy link
Contributor Author

@pacospace pacospace Sep 22, 2021

Choose a reason for hiding this comment

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

Thanks for the detailed explanation @fridex, now I understand :) and I think we can start with the boot then and when we will do something more complex later we can make it a warp (maybe when we analyze ps- images and learn specific versions per tags, so we can recommend more precise image tag), wdyt?

I agree with you on the use of "representatives"! I will modify the PR, thanks!!

@pacospace pacospace force-pushed the introduce-ps-stack-prescription branch from 8f7e5d3 to ee7a2ce Compare September 21, 2021 10:33
@sesheta sesheta added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 21, 2021
Signed-off-by: Francesco Murdaca <[email protected]>
@pacospace pacospace force-pushed the introduce-ps-stack-prescription branch from ee7a2ce to eeea6ee Compare September 21, 2021 15:21
@sesheta sesheta added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 21, 2021
@pacospace pacospace requested a review from fridex September 22, 2021 07:48
@sesheta sesheta added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 22, 2021
@pacospace pacospace force-pushed the introduce-ps-stack-prescription branch 2 times, most recently from c90dc0e to ca4d5ad Compare September 22, 2021 08:18
Signed-off-by: Francesco Murdaca <[email protected]>
@pacospace pacospace force-pushed the introduce-ps-stack-prescription branch from ca4d5ad to d3803df Compare September 22, 2021 08:19
Copy link
Contributor

@fridex fridex 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 👍🏻

@goern goern added the kind/demo This is an Issue or PR someone want to give a demo or request a demo. label Sep 22, 2021
@fridex
Copy link
Contributor

fridex commented Sep 22, 2021

/test pre-commit

@fridex
Copy link
Contributor

fridex commented Sep 22, 2021

Let's have this in 👍🏻 thanks!

@fridex fridex merged commit fccd45a into thoth-station:master Sep 22, 2021
@pacospace pacospace removed the kind/demo This is an Issue or PR someone want to give a demo or request a demo. label Jan 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants