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

Elixir Module support #6967

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

Elixir Module support #6967

wants to merge 42 commits into from

Conversation

wingyplus
Copy link
Contributor

@wingyplus wingyplus commented Mar 29, 2024

Let's try:

$ ./hack/dev
$ export PATH=$PWD/hack:$PWD/bin:$PATH
$ with-dev dagger init --sdk=github.com/dagger/dagger/sdk/elixir/runtime potato
Initialized module potato in potato
$ cd potato
$ with-dev dagger functions
Name             Description
container-echo   -
grep-dir         -
$ with-dev dagger call container-echo --string-arg 'Hello, Dagger' stdout
Hello, Dagger

🎉

Technical design

CAUTION: This is the long road to make it done. The design can be change depends on the discussion and feedback.

Let's take a look at module that SDK generate for you.

defmodule Potato do
  @moduledoc """
  A generated module for Potato functions.
  """

  use Dagger.Mod, name: "Potato"

  defstruct [:dag]

  @doc """
  Returns a container that echos whatever string argument is provided.
  """
  @function [
    args: [
      string_arg: [type: :string]
    ],
    return: Dagger.Container
  ]
  def container_echo(self, args) do
    self.dag
    |> Dagger.Client.container()
    |> Dagger.Container.from("alpine:latest")
    |> Dagger.Container.with_exec(~w"echo #{args.string_arg}")
  end

  @function [
    args: [
      directory_arg: [type: Dagger.Directory],
      pattern: [type: :string]
    ],
    return: :string
  ]
  def grep_dir(self, %{directory_arg: directory, pattern: pattern}) do
    self.dag
    |> Dagger.Client.container()
    |> Dagger.Container.from("alpine:latest")
    |> Dagger.Container.with_mounted_directory("/mnt", directory)
    |> Dagger.Container.with_workdir("/mnt")
    |> Dagger.Container.with_exec(["grep", "-R", pattern, "."])
    |> Dagger.Container.stdout()
  end
end

Any module that want to be a Dagger module needs to call use Dagger.Mod and need to declare the name of the module using :name option.

Each function has a module attribute called function. The value of that attribute is a keyword (for non-Elixir developer, think like a ordered key/value in another language) that contain the information of the function, currently has only argument and the return type. The module runtime will use this information to generate module declaration using Module API and do encode/decode value between Dagger and Elixir world.

The reason I declare this is because in Elixir, we have no other ways to obtain type from function argument because it is a dynamic programming language. But luckily that we can use data-structure to describe the function structure. :)

After declare the module, it's time to register a module. Take a look at application.ex inside dagger/potato/lib/potato

defmodule Potato.Application do
  # See https://hexdocs.pm/elixir/Application.html
  # for more information on OTP Applications
  @moduledoc false

  use Application

  @impl true
  def start(_type, _args) do
    children = [
      Dagger.ModuleRuntime.Registry, 
      Potato
    ]

    # See https://hexdocs.pm/elixir/Supervisor.html
    # for other strategies and supported options
    opts = [strategy: :one_for_one, name: Potato.Supervisor]
    Supervisor.start_link(children, opts)
  end
end

In Elixir, they has a concept called Supervisor which's a process (the lightweight process like a goroutine) that maintain processes defined in children to keep alive. We put it under Application and tell the Erlang/OTP to start this application when function start invoking.

In children, we have 2 process, the first one is module registry that store Dagger module information to uses at runtime, and the second one is the Dagger module implementation, it's Potato in the example above.

Tasks that need to done in this PR

  • Description support for module and function.
  • Support all primitive types. String already support but other types are not (integer, boolean, etc.).
  • Cleanup project code generator

Tasks that can follow up after this PR

  • Optional argument.
  • Field support (like @field in TypeScript). I still doubt how to do this at the moment. :(
  • Improve the documentation in Dagger.ModuleRuntime.
  • Decode object type back to struct.

`Dagger.ModuleRuntime` is a runtime for `Dagger` module for Elixir.
"""

def __on_definition__(env, :def, name, args, _guards, _body) do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To reviewer, the __on_definition__ invoke when Elixir start compile the source. When it found a function declaration, it'll pass the information to this function.

@gerhard
Copy link
Member

gerhard commented Apr 2, 2024

This looks amazing! WDYT @helderco @gmile?

What else is on your list @wingyplus before this is ready for the final review & merge?

@gmile
Copy link
Contributor

gmile commented Apr 2, 2024

Yes, I've been watching this PR for a while, this is really promising, thanks for ping @gerhard!

The kind of blocker I have with my article for Elixir SDK is entering a shell of the module to do interactive debugging inside, e.g. #4463 🤔 Is there anywhere I can read more about entering an interactive shell for a module? If not, is it because it's not possible yet in principle, or this kind of interactivity is not planned in the world of modules/functions?

To put it differently, let's say my dagger module runs mix test command, and it fails with a very, very weird error that's difficult to understand how to fix unless I'm in a shell, looking around inside the container file system, etc. - interactively.

@wingyplus
Copy link
Contributor Author

What else is on your list @wingyplus before this is ready for the final review & merge?

@gerhard I have add a checklist on the PR description. The checklist has some items that need to be done and some of it could follow-up later. The follow-up tasks may also convert to the issue after this PR ready to review.

BTW, the Elixir module is now working, if anyone have time to review, I'm really appreciate. :)

@wingyplus
Copy link
Contributor Author

The function doc is now supported like screenshot below.

Screenshot 2024-04-03 230144

If the plan is going well, this PR, the MVP version, can go out of draft version within next week. :)

@wingyplus
Copy link
Contributor Author

wingyplus commented Apr 5, 2024

Note for me:

  • add the limitation to the PR description

@wingyplus
Copy link
Contributor Author

cc @gmile If you want to try. :)

@wingyplus wingyplus marked this pull request as ready for review April 5, 2024 17:31
@wingyplus wingyplus requested a review from a team as a code owner April 5, 2024 17:31
@wingyplus
Copy link
Contributor Author

@gerhard @gmile @helderco The PR is ready to review now. 🎉

sdk/elixir/runtime/main.go Outdated Show resolved Hide resolved
@wingyplus wingyplus force-pushed the elixir-module branch 2 times, most recently from 64c1018 to 9d3eed1 Compare April 10, 2024 10:26

for _, name := range []string{"My-Module", "MyModule"} {
modGen := daggerCliBase(t, c).
With(daggerExec("init", "-vv", "--name="+name, "--sdk=elixir"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I remove -vv I got

=== NAME  TestModuleElixirInit/camel-cases_Dagger_module_name
    module_elixir_test.go:39:
                Error Trace:    /home/wingyplus/src/github.com/dagger/dagger/core/integration/module_elixir_test.go:39
                Error:          []string{".gitattributes", ".gitignore", "pyproject.toml", "requirements.lock", "sdk", "src"} does not contain "my_module"
                Test:           TestModuleElixirInit/camel-cases_Dagger_module_name
=== NAME  TestModuleElixirInit/with_source
    module_elixir_test.go:65:
                Error Trace:    /home/wingyplus/src/github.com/dagger/dagger/core/integration/module_elixir_test.go:65
                Error:          []string{".gitattributes", ".gitignore", "pyproject.toml", "requirements.lock", "sdk", "src"} does not contain "bare"
                Test:           TestModuleElixirInit/with_source
=== NAME  TestModuleElixirInit/from_scratch

Maybe because of I accidentally copy the python sdk at commit e3f97af. But the issue still occurred even correct the path. After try adding a flag, it works, change the directory name also works! 😢

@wingyplus wingyplus force-pushed the elixir-module branch 2 times, most recently from 4336dcb to 0f3abf1 Compare April 24, 2024 16:23
@gmile
Copy link
Contributor

gmile commented Apr 25, 2024

For what it's worth, I tested this branch few times now, all is working as expected, no complains from here. The biggest thing I was waiting is terminal sub-command, and it works like a charm. HUGE thank you for all the hard work, @wingyplus! 🥇👏 🎉

@wingyplus wingyplus mentioned this pull request Apr 26, 2024
7 tasks
Copy link
Contributor

@helderco helderco left a comment

Choose a reason for hiding this comment

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

Sorry for taking such a long time, I'm not at all familiar with Elixir. Took most notes on the runtime module. Since it's an experimental SDK, we can do more incremental improvements so my comments aren't merge blockers (except for one).

There's several tests in core/integration/module_test.go that put several SDKs under the same test, you just have to add the equivalent elixir code as a test case. For example:

func TestModuleGit(t *testing.T) {

func TestModuleDescription(t *testing.T) {

func TestModuleOptionalDefaults(t *testing.T) {

See how many you can add. It's an easy way to check for consistency and add test coverage.

sdk/elixir/runtime/main.go Outdated Show resolved Hide resolved
sdk/elixir/runtime/main.go Outdated Show resolved Hide resolved
sdk/elixir/runtime/main.go Outdated Show resolved Hide resolved
sdk/elixir/runtime/main.go Outdated Show resolved Hide resolved
sdk/elixir/runtime/main.go Outdated Show resolved Hide resolved
sdk/elixir/runtime/main.go Outdated Show resolved Hide resolved
sdk/elixir/runtime/main.go Outdated Show resolved Hide resolved
sdk/elixir/runtime/main.go Outdated Show resolved Hide resolved
sdk/elixir/runtime/main.go Outdated Show resolved Hide resolved
sdk/elixir/runtime/template.exs Outdated Show resolved Hide resolved
@wingyplus
Copy link
Contributor Author

Thanks for reviewing @helderco. That was a huge comments! I'll take a look during this weekend. :)

@wingyplus
Copy link
Contributor Author

@helderco Unfortunately, I can do only one integration tests because most of it required to have struct supported, which's not covered by this PR. And found test TestModuleDescription is not played well with the current approach because Elixir mix new will prompt if it detect any file(s) in the directory that needs to be created.

Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
This accident get change by
dagger@e4cd936
but it get revert after rebased. This commit bring that changes back.

Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
* Run `dagger develop` against dagger version 0.11.2.
* Eliminate passing `dag` variable to function.
* Add digest to Elixir image.

Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
* Moved all constant call always on top of mounting module source that's
  change often.
* Refactor to `dagger` sdk workdir.
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Move codegen out of the container and generate code on separate
container.

Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
… digit

Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>
@wingyplus
Copy link
Contributor Author

Hmmm... how do you pass the sdk's files when not a built-in runtime module? Have you been able to make it work?

@helderco That's my mistake 🙏 . Fixed by clone it in New function instead but the url still invalid because I used github.com/dagger/dagger on branch main which's currently not available at the moment, we need to test after merge this PR.

If you have any suggestion, feel free to comment.

@wingyplus
Copy link
Contributor Author

@wingyplus, can you update instructions in the description now that it's not a built-in module?

Done. Also added to the README.md but it's need a little polish after merge.

@helderco
Copy link
Contributor

helderco commented May 22, 2024

[...] I used github.com/dagger/dagger on branch main which's currently not available at the moment, we need to test after merge this PR.

PRs have their own URL: https://github.com/dagger/dagger#pull/6967/head

See https://play.dagger.cloud/playground/COGeM4i4Z36

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.

None yet

4 participants