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/context directory #7744

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

TomChv
Copy link
Member

@TomChv TomChv commented Jun 24, 2024

API changes

This PR updates the Function and FunctionArg from our engine API to support defaultPathFromContext

# Function

  withArg(
    """
    If the argument is a Directory or File type, default to load path from context directory, relative to root directory.
    """
    defaultPathFromContext: String = ""
# }

# FunctionArg {

  """
  If the argument is a Directory or File type, default to load path from context directory, relative to root directory.
  """
  defaultPathFromContext: String!

# }

Engine changes

  • Update Call to load contextual values and insert it as input.
  • Add a loadContextualArg function that take the typedef and retrieve its contextual default value.
  • Add a function loadContext to load from git or local source the contextual path.
    This implements the spec designed in Context directory #7647
    For git: we load it from the context directory since it includes the whole repo.
    For local: we load it from importLocal with the specific directory specified so we never load the full repo expect for /

Current progress

  • Loading directory
  • Get *Directory to ID: I found a workaround with Blob but it's not okay for File, I will check if I can use select instead.
  • Load File, using Select actually works!
  • Handle relative path
  • Write tests
  • Complete all SDK support

Spec Reminder

  • The context directory is the git repository containing the module. If there is not git repository, the context is simply the module.
  • Relative paths are relative to the current source file, scoped to the context directory for security
  • Absolute paths are rooted in the context directory
  • There is no limit to the number of directories which can have default values of this sort
  • Optionally, directories can be filtered with a gitignore-style annotation

SDKs support

  • Go using pragmas directive on Directory or File type ⌛

  • Typescript using @context("path")
    Example

@func()
build(
  @context("./frontend/src") frontend: Directory,
  @context("./backend/app/src") backend: Directory,
  @context("./tools/builder.conf") buildConfig: File,
): Directory {}
  • Python using annotations

Fixes #7647

@TomChv TomChv self-assigned this Jun 24, 2024
@sipsma
Copy link
Contributor

sipsma commented Jun 25, 2024

(continuing a conversation from discord)

For now, I think the way to general way to approach this feature should be to leave the current module context loading as is since it's used by the SDKs to build modules and then add this support for user arg context dirs as separate context loading calls. There's likely ways of unifying and optimizing this but better to start simple for now.

There's different ways of approaching this, I'm just gonna outline the first reasonable possibility that comes to mind. Not set in stone at all though so happy to hear other opinions and happy to help out more if reality of implementing it doesn't align with theory.

  1. (discussed before) ModuleFunction has access to the Module and ModuleSource via this field: https://github.com/sipsma/dagger/blob/5231aa7de19266430581da054604ce533cc85898/core/modfunc.go#L25-L25
  2. Add a new method to ModuleSource called something like LoadContext(path: string) (dagql.Instance[*Directory], error) that allows you to dynamically load dirs from the module source context.
    • This would be different than the existing ContextDirectory method that returns the static one loaded for the SDKs (we can consider renaming it maybe for clarity).
    • The way that you implement loading the context depends on whether it's coming from a remote git repo or a local git repo.
      • git is easy because we haven't implemented fancy dynamic loading, we just have to pull the whole repo, so you can just copy the directory from it's already loaded context dir
      • local is more involved since you will need to actually load it on demand. You can look at this code and the methods it calls to get an idea of how to do that (one of the really key parts being this section that does the load itself)
        • Another thing to be aware of is that the LocalImport call is sensitive to values passed in ctx which determine which client the dir should be loaded from. For now, we can just load from the "main client caller". I already wrote too much here, so just ping if you need pointers on this 🙂
  3. Once that's in place, you can plug the arg in as a directory value same as others are (i.e. in this section)

@TomChv
Copy link
Member Author

TomChv commented Jun 25, 2024

This would be different than the existing ContextDirectory method that returns the static one loaded for the SDKs (we can consider renaming it maybe for clarity).

I understand this but this directory will not have the full repo if it's not part included in include, how do I workaround that?

@sipsma
Copy link
Contributor

sipsma commented Jun 26, 2024

This would be different than the existing ContextDirectory method that returns the static one loaded for the SDKs (we can consider renaming it maybe for clarity).

I understand this but this directory will not have the full repo if it's not part included in include, how do I workaround that?

You would need to add a new method that does the actual load of files from the original context directory (i.e. loads from the user's local dirs or loads from the git repo). That's what I was referring to under "The way that you implement loading the context depends on whether it's coming from a remote git repo or a local git repo." in my previous comment

@TomChv
Copy link
Member Author

TomChv commented Jun 26, 2024

Relative paths are relative to the current source file, scoped to the context directory for security

To do such thing, we need to know the file where the module func comes from, I'm not sure we got this information in the current API.
I think the resolution should be done by the SDK, what do you think @helderco @jedevc?

@shykes
Copy link
Contributor

shykes commented Jun 26, 2024

Relative paths are relative to the current source file, scoped to the context directory for security

To do such thing, we need to know the file where the module func comes from, I'm not sure we got this information in the current API. I think the resolution should be done by the SDK, what do you think @helderco @jedevc?

Sorry I forgot to update that line after further discussion. It should be relative to the module root, not the file (that would be too difficult to find out).

@TomChv
Copy link
Member Author

TomChv commented Jun 26, 2024

Sorry I forgot to update that line after further discussion. It should be relative to the module root, not the file (that would be too difficult to find out).

Ohh then yeah it's much easier haha

@TomChv
Copy link
Member Author

TomChv commented Jun 26, 2024

Another question @helderco @jedevc @shykes, should the API returns an error during registration if the typedef has both a default value and a contextual one?
This is possible in TypeScript and Python due to the notation so should this results in an error? I think so but I want your opinion.

Example of notation:

@func()
 async test(@context("/") dir: Directory = dag.directory()): Promise<string[]>

@shykes
Copy link
Contributor

shykes commented Jun 26, 2024

should the API returns an error during registration if the typedef has both a default value and a contextual one? This is possible in TypeScript and Python due to the notation so should this results in an error? I think so but I want your opinion.

I thought we were using default values? I might have missed (or forgotten) the last round of bikeshedding on that topic.

@TomChv
Copy link
Member Author

TomChv commented Jun 26, 2024

I thought we were using default values? I might have missed (or forgotten) the last round of bikeshedding on that topic.

No it's a specific field in the API, see the change of spec in the description

@shykes
Copy link
Contributor

shykes commented Jun 26, 2024

@TomChv could you add example snippets of what that feature would look like, in the 3 main languages please? It would make it easier to discuss the DX tradeoffs

@helderco
Copy link
Contributor

I thought we were using default values? I might have missed (or forgotten) the last round of bikeshedding on that topic.

I updated my view in #7647 (comment):

[...] warming up to not using the default value because of that as it makes an overall simpler implementation, just sacrificing on DX for SDKs that support default values, but that can be improved later.


  • Typescript using @context("path") ⚡ to be tested once the engine changes are completed
  • Python using @context("path") ? To be confirmed by @helderco

We need to bikeshed on the @context name.

In Python, this would have to be done using metadata annotation on the type:

def build(
    self,
    backend: Annotated[
        dagger.Directory,
        DefaultFromContext("../backend/app/src"),
        Ignore(...),
    ]
) -> dagger.Directory:
    ...

@TomChv
Copy link
Member Author

TomChv commented Jun 26, 2024

@TomChv could you add example snippets of what that feature would look like, in the 3 main languages please? It would make it easier to discuss the DX tradeoffs

I think it should look like this:

Go

func (app *App) Build(
  // +default="./frontend/src"
  frontend *Directory,
  // +default="./backend/app/src"
  backend *Directory,
  // +default="/tools/builder.conf"
  buildConfig *File,
) *Directory {}

Typescript

@func()
build(
  @context("./frontend/src") frontend: Directory,
  @context("./backend/app/src") backend: Directory,
  @context("./tools/builder.conf") buildConfig: File,
): Directory {}

Python

Done by Helder above

def build(
  self,
  backend: Annotated[
     dagger.Directory,
     DefaultFromContext("../backend/app/src"),
      Ignore(...),
  ]
) -> dagger.Directory:
  ...

@helderco
Copy link
Contributor

In Go, rather than // +context, it should be // +default.

@shykes
Copy link
Contributor

shykes commented Jun 26, 2024

@TomChv could you add an example of how regular default values work in each case?

@TomChv
Copy link
Member Author

TomChv commented Jun 26, 2024

@TomChv could you add an example of how regular default values work in each case?

Example on https://docs.dagger.io/manuals/developer/functions#default-values

Go

I'm not sure you can default to a directory value in Go so I don't know??

But for primitive type, you set a pragma like +default=4

Typescript

@func()
build(
  frontend: Directory = dag.directory(),
  backend: Directory = dag.directory(),
  buildConfig: File = dag.directory().WithNewFile("xxx", "xxx").File("xxx"),
): Directory {}

Python

I'm not sure about the syntax but probably:

def build(
  self,
  backend: Annotated[
     dagger.Directory,
  ] = dag.directory()
) -> dagger.Directory:
  ...

@shykes shykes mentioned this pull request Jun 26, 2024
@TomChv TomChv marked this pull request as ready for review June 26, 2024 23:21
@TomChv TomChv requested review from a team as code owners June 26, 2024 23:21
@jedevc jedevc added the needs/changelog Pull requests that require a changelog entry label Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/changelog Pull requests that require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Context directory
5 participants