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

feat: Better envdlib syntax support #1132

Open
aseaday opened this issue Nov 1, 2022 · 7 comments
Open

feat: Better envdlib syntax support #1132

aseaday opened this issue Nov 1, 2022 · 7 comments

Comments

@aseaday
Copy link
Member

aseaday commented Nov 1, 2022

Description

When I am working on writing envdlib for supporting tensorRT, I found some points we need for better use of envdlib:

  • Branch Judgment: we need judge wether current condition like os version and cuda version supported by tensorRT
  • Print: sometimes we should print some information for user's to use such as the location of sdk or agreement default to accept.
  • Context Varibles: we need context varibales such as os version to decide how to install

We still need to some work to implement composable image building. We still use the process-oriented pattern to acheive my goal. Maybe it is not a good idea in the future. https://nixos.org/ can give us some guides.


Message from the maintainers:

Love this enhancement proposal? Give it a 👍. We prioritise the proposals with the most 👍.

@gaocegege
Copy link
Member

/cc @kemingy

@kemingy
Copy link
Member

kemingy commented Nov 1, 2022

* Branch Judgment: we need judge wether current condition like os version and cuda version supported by tensorRT

* Print: sometimes we should print some information for user's to use such as the location of sdk or agreement default to accept.

I'm wondering if the functions can return some kind of structure. But surely that will break the current process-oriented pattern.

* Context Varibles: we need context varibales such as os version to decide how to install

To pass the information to users, I think we need to expose the Graph (or something equivalent).

@aseaday aseaday closed this as completed Nov 4, 2022
@aseaday aseaday moved this to Done in envd sprint board Nov 4, 2022
@aseaday aseaday reopened this Nov 4, 2022
@VoVAllen VoVAllen self-assigned this Nov 7, 2022
@VoVAllen
Copy link
Member

VoVAllen commented Nov 9, 2022

Generally, this means function can share information across different function.

Solution 1:
Use global variable-like mechanism
For example, using provide and inject (idea from vue3)

def cuda(version="11.6.2"):
    provide("cuda/version", "11.6.2")

def torch():
    # inject(key, default_value)
    cuda_version = inject("cuda/version", "10.2") 
    install.python_package(["torch-cu{}".format(cuda_version)])

Then user can directly call envdlib.torch() to install the version related to cuda version. Note the execution needs to be ordered, user have to execute install.cuda before envdlib.torch

Solution 2:
Expose the underlying build graph

def torch():
    # _get_build_graph convert build graph into python dict
    info_dict = _get_build_graph()
    version = info_dict["cuda"]

@kemingy
Copy link
Member

kemingy commented Nov 9, 2022

Solution 1: Use global variable-like mechanism For example, using provide and inject (idea from vue3)

def cuda(version="11.6.2"):
    provide("cuda/version", "11.6.2")

def torch():
    # inject(key, default_value)
    cuda_version = inject("cuda/version", "10.2") 
    install.python_package(["torch-cu{}".format(cuda_version)])

I feel defining a global variable could be easier.

cuda_version = "11.6.2"

def build():
    install.cuda(cuda_version)
    envdlib.tensorrt(cuda=cuda_version)

We should document the if-else, for, string, print (debug maybe), etc.

Ref: https://github.com/bazelbuild/starlark/blob/master/spec.md

@VoVAllen
Copy link
Member

VoVAllen commented Nov 9, 2022

@kemingy Arbitrary global variable doesn't seem good. At least a global dictionary needed I think. such as we have a global variable such as global_info and user can do anything they like on it

@kemingy
Copy link
Member

kemingy commented Nov 9, 2022

@kemingy Arbitrary global variable doesn't seem good. At least a global dictionary needed I think. such as we have a global variable such as global_info and user can do anything they like on it

I mean users can define their own global variables to use.

It seems users don't know what starlark can do. And how to combine starlark built-in features with envd functions.

@aseaday
Copy link
Member Author

aseaday commented Nov 9, 2022

I think we'd better give a spec for envdlib's contributor such as:

  • how to define/get metadata need or provide
  • print or debug some information

But for normal users, we could just let them know they can treat starlark like python and builtin functions list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

No branches or pull requests

4 participants