feat(sdk/elixir): rework on Elixir codegen #6559

wingyplus · 2024-02-01T16:42:03Z

This is a big change but let me explain to catch up easier.

Codegen moved out from the SDK

Previously, the dagger codegen was embedded in the SDK, this caused a problem in that it could not be run when the codegen generated a bad code. This change introduces a new Elixir module called dagger_codegen which's responsible for generating code and uses the escript to build a binary instead of the mix task.

The way to fetch introspection is now split out of the codegen. This way can reduce the compilation annoying issue, described in #5901, but the issue still happens when using hack/make sdk:elixir:generate.

`sdk:elixir` tasks improvement

The hack/make sdk:elixir:* task is now upgraded to use Elixir 1.16 with Erlang/OTP 26. The sdk:elixir:generate will not produce a bad code when its failure.

Nicer code-generation

Previously, the codegen used Elixir quote to produce code but it's not suitable for our use case, the codegen is a bit hard to read and needs to deal with complex AST structure. The new codegen uses a new approach by using EEx as a template and moving some complex code-generation to the renderer module.

The typespec and documentation also improved.

Here is comparison

0.9.7

From this PR

Fix issues in the codegen

This temporarily fixes issue 🐞 Elixir/Rust/Python: strange experimentalWithAllGPUs function name #6310 about the acronym word format from camel to snake case.
Fix 🐞 Elixir codegen didn't get cleanup when generate failure #5901 by do not export a bad code during run sdk:elixir:generate as mentioned above.
Fix removed APIs not get cleanup when run sdk:elixir:generate.

Cleanup

Eliminate Nestru out of the codegen since we change how to query list object like envVariables.
Takedown the codegen in lib/dagger/codegen.

Improvement on the next PR

Improve test coverage on codegen.
Make codegen easier to write. Currently, some of it is living inside the template but some of it looks complex living inside the module.
Make required arguments more safety with guards.

wingyplus · 2024-02-05T15:48:10Z

Found the env_variables API broken (#6594). We need to rework the simple object also. 😢

wingyplus · 2024-02-16T17:07:36Z

sdk/elixir/lib/dagger/gen/container.ex

+    with {:ok, items} <- execute(selection, container.client) do
+      {:ok,
+       for %{"id" => id} <- items do
+         %Dagger.EnvVariable{
+           selection:
+             query()
+             |> select("loadEnvVariableFromID")
+             |> arg("id", id),
+           client: container.client
+         }
+       end}
+    end


Any function that return a list of object are breaking changes since version 0.9.7 but didn't notice user about this breaking changes. Do we need to add a changelog about this?

github-actions · 2024-03-14T01:46:48Z

This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

wingyplus · 2024-03-20T16:50:18Z

internal/mage/sdk/elixir.go

 		WithServiceBinding("dagger-engine", devEngine).
 		WithEnvVariable("_EXPERIMENTAL_DAGGER_RUNNER_HOST", endpoint).
 		WithMountedFile(cliBinPath, cliBinary).
 		WithEnvVariable("_EXPERIMENTAL_DAGGER_CLI_BIN", cliBinPath).
-		WithExec([]string{"mix", "dagger.gen"})
+		WithExec([]string{"mix", "run", "scripts/fetch_introspection.exs"}).


I need to extract introspection fetching out of the codegen to make reusable with the module (See https://github.com/wingyplus/daggerverse/blob/main/elixir-sdk/main.go#L90-L100).

It would be nice if mage tasks (or incoming ci module) could inject an introspection JSON file and let the codegen use that file.

It would be nice if mage tasks (or incoming ci module) could inject an introspection JSON file and let the codegen use that file.

That's done in modules, more specifically via the SDK runtime module. The server provides the introspection JSON and so, codegen needs to be able to read from that.

An external way to get it is to:

dagger -m github.com/helderco/daggerverse/codegen@v0.1.2 call introspect as-json -o introspection.json

In our CI module that can be a dependency so you get it with:

introspectionJson := dag.Codegen().Introspect().AsJson() // a dagger.File

The idea with this module is to eventually use it as a common ground for SDKs to codegen, without reinventing the wheel:

Improve codegen reusability #5226

https://github.com/helderco/daggerverse/blob/codegen/v0.1.2/codegen/README.md

That's done in modules, more specifically via the SDK runtime module. The server provides the introspection JSON and so, codegen needs to be able to read from that.

Yeah, I see it from the module runtime. So I thought it would be nice to have this included in generate task on non-support module SDK (rust, java, elixir, etc.).

An external way to get it is to:

dagger -m github.com/helderco/daggerverse/codegen@v0.1.2 call introspect as-json -o introspection.json

Ahh, so we can calling the function using cliBinPath to download introspection and then using it to generate code. Nice! Do we have a plan to include it in new CI module (#6843)?

No plan to put it in the CI module as of yet, but you can do it for Elixir 🙂

wingyplus · 2024-03-20T16:55:26Z

The PR size is quite big now. So I decided to stop on this and continue on a new PR for incoming improvements.

jedevc · 2024-03-25T10:14:18Z

The PR size is quite big now. So I decided to stop on this and continue on a new PR for incoming improvements.

@wingyplus what do you want to do about this PR? Should it be reviewed/merged, or do you want to split it into smaller ones? Either works for us.

wingyplus · 2024-03-25T15:15:48Z

The PR size is quite big now. So I decided to stop on this and continue on a new PR for incoming improvements.

@wingyplus what do you want to do about this PR? Should it be reviewed/merged, or do you want to split it into smaller ones? Either works for us.

Review and merge is the way I prefer. For the mage task changes, I'll update on #6843 to make it up-to-date if this PR merge before that PR. 🙇

wingyplus · 2024-03-25T15:23:57Z

Rebased.

gerhard · 2024-03-26T10:55:00Z

This is on my list for today.

If we can resolve the conflicts before then, merging this will be straigthforward.

Want to take a look too @gmile?

gerhard

This is a HUGE PR @wingyplus 😱 My apologies for not getting to it earlier.

This looks great to me 💪 I am keen to get it moving & so that you can follow-up with the rest. I trust that you will get the merge conflicts solved & add any new tests if needed (looks good to me as is).

In case @gmile and @helderco want to check it out too, please do, otherwise I'm excited for the follow-ups 🙌

FWIW, we - especially @jedevc - are communicating upcoming releases via https://github.com/dagger/dagger/milestones, so you can see what release is coming up, and when.

gerhard · 2024-03-26T11:18:32Z

sdk/elixir/dagger_codegen/lib/dagger/codegen/introspection/types/type_ref.ex

+
+  def is_list_of?(_, _), do: false
+
+  # TODO: refactor me.


gerhard · 2024-03-26T11:21:03Z

sdk/elixir/lib/dagger/core/query_builder.ex

@@ -18,12 +18,23 @@ defmodule Dagger.Core.QueryBuilder.Selection do
    }
  end

-  def arg(%__MODULE__{args: args} = selection, name, value) when is_binary(name) do
+  # TODO: Remove me.


Did you mean to remove this now, or in a follow-up PR?

I need to tackle it on the PR while I rework on the query builder.

gmile

Amazing work! 👏🏅 Looking forward to having this merged!

gmile · 2024-03-26T11:58:53Z

sdk/elixir/dagger_codegen/mix.exs

+    [
+      app: :dagger_codegen,
+      version: "0.1.0",
+      elixir: "~> 1.15",


In internal/mage/sdk/elixir.go an earlier version is mentioned, 1.14.5. Should these be in sync?

Changing it to 1.14 would align with the main package. 👍

Fixed on the current commit.

helderco

Ok to iterate on follow-ups, so approving now. 🚀

sdk/elixir/scripts/fetch_introspection.exs

internal/mage/sdk/elixir.go

sdk/elixir/dagger_codegen/lib/dagger/codegen/elixir_generator/formatter.ex

helderco · 2024-03-26T13:33:54Z

sdk/elixir/dagger_codegen/lib/dagger/codegen/elixir_generator/renderer.ex

+  def render_deprecated(%Field{deprecation_reason: ""}), do: ""
+  def render_deprecated(%Field{deprecation_reason: nil}), do: ""
+
+  def render_deprecated(%Field{deprecation_reason: reason}) do


We have a convention that deprecation reason messages can reference a field (most likely the field that should be used instead), wrapped in backticks (e.g., use `newField` instead). The reason for this is that SDKs can extract whatever's inside the backticks from the deprecation reason and re-format it according to its own naming conventions.

Example in Python:

dagger/sdk/python/src/dagger/_codegen/generator.py

Lines 664 to 673 in 795a735

def deprecated(self, prefix='"', suffix='"') -> str:

def _format_name(m):

name = format_name(m.group().strip("`"))

return f"{prefix}{name}{suffix}"

return (

DEPRECATION_RE.sub(_format_name, reason)

if (reason := self.graphql.deprecation_reason)

else ""

)

dagger/docs/docs-graphql/schema.graphqls

Line 2142 in 795a735

file(id: FileID!): File! @deprecated(reason: "Use `loadFileFromID` instead.")

dagger/sdk/python/src/dagger/client/gen.py

Line 5696 in 795a735

Use :py:meth:`load_file_from_id` instead.

Currently, I've done field conversion by transform content in backquote into snakecase. But the hard part is to link to referenced function in Elixir, we need the arity which's the number of argument of that function. So it's now become a little bit tricky that we need to traverse introspection to find the exact number. 😢

I think it would be great to follow up this issue on the next PR.

Reference to issue #6939

Oh, now I see that's replaced in all descriptions, not just deprecations. We've avoided referencing other fields in the description in the core API because of the wrong naming convention in other languages, but I think that's no longer the case. It's easy to creap in.

We've limited this to deprecation reasons because it's more common in field descriptions to want something in backquotes other than being a field name. Doesn't seem like there's a case like this today though, but that can change. It's smart to only match valid identifiers, but I can't rule out a name in backquotes that is a valid identifier, but meant for something else other than an API field.

Changing the casing is enough. When I said "reference", I wasn't referring to anything special in code, even though Python does generate a prefix that allows it to be hyperlinked correctly in the reference docs. But the convention here is just to rename, having IDE autocomplete in mind.

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

wingyplus · 2024-03-26T16:56:53Z

I think this PR is in a good shape now. So I'm gonna merge this now.

Thank you @gerhard @helderco @gmile for helping review this PR.

Follow-up conversation in dagger#6559 (comment). The function in backquote should convert to snake case only deprecation message. Fixes dagger#6939 Signed-off-by: Thanabodee Charoenpiriyakij <wingyminus@gmail.com>

wingyplus force-pushed the elixir-new-codegen branch 2 times, most recently from 8a815f7 to c385d3d Compare February 5, 2024 15:25

wingyplus force-pushed the elixir-new-codegen branch 11 times, most recently from 6f57364 to 9dbc689 Compare February 16, 2024 16:55

wingyplus marked this pull request as ready for review February 16, 2024 16:58

wingyplus requested a review from a team as a code owner February 16, 2024 16:58

wingyplus commented Feb 16, 2024

View reviewed changes

wingyplus force-pushed the elixir-new-codegen branch 2 times, most recently from 24c9541 to b1ecff5 Compare February 19, 2024 03:42

wingyplus mentioned this pull request Feb 19, 2024

🐞 dagger init returns internal server erroro when inititate with git sdk #6689

Closed

wingyplus force-pushed the elixir-new-codegen branch 10 times, most recently from 7776fbc to a895daa Compare February 27, 2024 15:49

wingyplus force-pushed the elixir-new-codegen branch from d52edcf to 850b751 Compare February 28, 2024 16:27

github-actions bot added the stale label Mar 14, 2024

wingyplus removed the stale label Mar 14, 2024

wingyplus force-pushed the elixir-new-codegen branch 2 times, most recently from 519f3a6 to c200e2e Compare March 20, 2024 16:45

wingyplus commented Mar 20, 2024

View reviewed changes

wingyplus force-pushed the elixir-new-codegen branch from c200e2e to 156714c Compare March 22, 2024 06:29

wingyplus force-pushed the elixir-new-codegen branch from 156714c to 7abcbd8 Compare March 25, 2024 15:12

wingyplus force-pushed the elixir-new-codegen branch from 7abcbd8 to 5fadf0f Compare March 25, 2024 15:23

wingyplus force-pushed the elixir-new-codegen branch from 5fadf0f to c5aa758 Compare March 25, 2024 15:32

gerhard approved these changes Mar 26, 2024

View reviewed changes

gmile reviewed Mar 26, 2024

View reviewed changes

helderco approved these changes Mar 26, 2024

View reviewed changes

wingyplus force-pushed the elixir-new-codegen branch from c5aa758 to 8e0e5ad Compare March 26, 2024 15:18

feat(sdk/elixir): rework on Elixir codegen

ddb74a4

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

wingyplus force-pushed the elixir-new-codegen branch from 8e0e5ad to ddb74a4 Compare March 26, 2024 16:01

wingyplus merged commit d15fb8b into dagger:main Mar 26, 2024
43 checks passed

wingyplus deleted the elixir-new-codegen branch March 26, 2024 16:57

wingyplus mentioned this pull request Mar 26, 2024

Modularize CI #6843

Merged

13 tasks

wingyplus mentioned this pull request Apr 20, 2024

✨ Elixir: format function in backquote only deprecation message #6939

Closed

wingyplus mentioned this pull request Apr 20, 2024

fix(sdk/elixir): format function only deprecation message #7148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sdk/elixir): rework on Elixir codegen #6559

feat(sdk/elixir): rework on Elixir codegen #6559

wingyplus commented Feb 1, 2024 •

edited

wingyplus commented Feb 5, 2024

wingyplus Feb 16, 2024

github-actions bot commented Mar 14, 2024

wingyplus Mar 20, 2024 •

edited

helderco Mar 26, 2024 •

edited

wingyplus Mar 26, 2024

helderco Mar 26, 2024 •

edited

wingyplus commented Mar 20, 2024

jedevc commented Mar 25, 2024

wingyplus commented Mar 25, 2024

wingyplus commented Mar 25, 2024

gerhard commented Mar 26, 2024

gerhard left a comment •

edited

gerhard Mar 26, 2024

gerhard Mar 26, 2024

wingyplus Mar 26, 2024

gmile left a comment

gmile Mar 26, 2024

wingyplus Mar 26, 2024

wingyplus Mar 26, 2024

helderco left a comment

helderco Mar 26, 2024

wingyplus Mar 26, 2024

wingyplus Mar 26, 2024

helderco Mar 26, 2024

wingyplus commented Mar 26, 2024

	def deprecated(self, prefix='"', suffix='"') -> str:
	def _format_name(m):
	name = format_name(m.group().strip("`"))
	return f"{prefix}{name}{suffix}"

	return (
	DEPRECATION_RE.sub(_format_name, reason)
	if (reason := self.graphql.deprecation_reason)
	else ""
	)

feat(sdk/elixir): rework on Elixir codegen #6559

feat(sdk/elixir): rework on Elixir codegen #6559

Conversation

wingyplus commented Feb 1, 2024 • edited

Codegen moved out from the SDK

sdk:elixir tasks improvement

Nicer code-generation

Fix issues in the codegen

Cleanup

Improvement on the next PR

wingyplus commented Feb 5, 2024

Choose a reason for hiding this comment

github-actions bot commented Mar 14, 2024

wingyplus Mar 20, 2024 • edited

Choose a reason for hiding this comment

helderco Mar 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helderco Mar 26, 2024 • edited

Choose a reason for hiding this comment

wingyplus commented Mar 20, 2024

jedevc commented Mar 25, 2024

wingyplus commented Mar 25, 2024

wingyplus commented Mar 25, 2024

gerhard commented Mar 26, 2024

gerhard left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helderco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wingyplus commented Mar 26, 2024

wingyplus commented Feb 1, 2024 •

edited

`sdk:elixir` tasks improvement

wingyplus Mar 20, 2024 •

edited

helderco Mar 26, 2024 •

edited

helderco Mar 26, 2024 •

edited

gerhard left a comment •

edited