Repository navigation
TS SDK: name a task's inputs, and take its id from the handler - #73437
jason810496 merged 1 commit into
Conversation
d7a6936 to
dc92410
Compare
e3f5dc3 to
0c3b08b
Compare
0c3b08b to
adb4c2e
Compare
pierrejeambrun
left a comment
There was a problem hiding this comment.
Nice thanks.
A few minor comments
|
@ashb TS SDK: Retrieve task id from fn name via a plugin to manipulate the source before packaging. Since this adds some complexity, I'm second guessing if keepNames (basically not minifying names) was a best option
Also common node recommendation for server side is typically not to minify at all. (keepNames can meet best of both world).
My bad |
There was a problem hiding this comment.
Also common node recommendation for server side is typically not to minify at all.
By this way and from your perspective, should we support the tsx way to start the TS SDK runtime?
Since Ash and I had a conversation back in Airflow Submit that Ash would like to have a way to start the TS runtime without bundling. I'm not sure the convention of "bundling or not" for TS server-side project myself.
However, all the current SDKs are heavily rely on the metadata at the artifacts for discovery purpose.
Since this topic is related with this PR so I would like to bring this up.
|
(Sorry for the incorrect ping Andrew) |
We can add this alongside current capability if this helps our users.
For portability reason i'd say that we still want bundling to happen. (pnpm install + moving whole source file around is probably not great) |
adb4c2e to
1fa5029
Compare
1fa5029 to
8fb2d5f
Compare
jason810496
left a comment
There was a problem hiding this comment.
Heads up on one consequence:
Without the plugin, the task id and the argument names are read off the handler when the Dag module "runs" runtime.
The "Runs" here is not the task runtime. It is the Dag module executes during packing, where airflow-ts-pack runs the staged bundle with --airflow-metadata (node bundle.min.mjs --airflow-metadata) to collect the dag ids and task ids for the embedded manifest.
Not sure would it still be more clean if we implement the pack time plugin as last round to retrieve the function and arguments names as TaskSpec.
The design choice between Packing transformation vs Runtime inferring
pierrejeambrun
left a comment
There was a problem hiding this comment.
LGTM, two comments we need to address before moving forward.
8fb2d5f to
f86a495
Compare
f86a495 to
ea3d111
Compare
jason810496
left a comment
There was a problem hiding this comment.
Thanks for the review and the discussion.
I just addressed all the comments.
ea3d111 to
851c98d
Compare
A native Dag handler takes one object of named arguments, and a call names each input, so nothing labels an argument arg0. A call given more than one argument is an error rather than a silently dropped value. The task id defaults to the handler's function name, so airflow-ts-pack passes esbuild's keepNames to keep that name through minification.
851c98d to
bd3594f
Compare
mainon its own.Why
Every argument reaches the serialized Dag by name, and a positional parameter list has none unless the SDK reads them out of the handler's source. A handler that takes one object of named arguments is what a TypeScript library takes anyway, and it settles both ends: the call site carries the names, and the handler's own name is still the task id.
How
arg0.TaskOptions.argBindingsis gone with it.dag.task(handler)takes its id fromhandler.name, andTaskSpec.taskIdsets one for an anonymous handler. Giving the id twice is an error rather than a silent winner.airflow-ts-packpasses esbuild'skeepNames, so the id survives minification.ts-sdk/adr/0002records the positional handler as tried and dropped. An earlier revision of this PR read the names at pack time with an esbuildonLoadhook and TypeScript's parser; it needed a parser in the packer and could not see a handler declared in another module.Was generative AI tooling used to co-author this PR?