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

[dagster-k8s] per op config fails on dynamic op graph #26588

Closed
abhinavDhulipala opened this issue Dec 19, 2024 · 0 comments · Fixed by #26670
Closed

[dagster-k8s] per op config fails on dynamic op graph #26588

abhinavDhulipala opened this issue Dec 19, 2024 · 0 comments · Fixed by #26670
Labels
deployment: k8s Related to deploying Dagster to Kubernetes type: bug Something isn't working

Comments

@abhinavDhulipala
Copy link
Contributor

abhinavDhulipala commented Dec 19, 2024

What's the issue?

Using the new per_step_k8s_config with dynamic ops should be possible by simply using the op name for configuration. Currently, the step_key is used to index into the config which has the format op_name[mapping_key]. The code suggests(per_op_override) that the intended result was to simply index with op_name

Example:

Given the following job

@job
def dynamic_producer_consumer_job():
    @op(out=DynamicOut(int))
    def dyn_producer():
    for i in range(3):
        return DynamicOutput(i, str(i))
    @op
    def dyn_sink(producer: int): ...

What did you expect to happen?

We expect the following execution config to propagate resource configs to every dyn_sink[0-2] op:

execution:
  config:
    step_k8s_config:
      dyn_sink:
         container_config:
           resources:
             requests:
               cpu: '300m'

Instead, this will just get ignored.

How to reproduce?

I can provide a unit test in a PR to fix this, but essentially take the job & config above and either kubectl describe po <run pod> or observe the K8sContainerContext from the K8sStepHandler and see the configs not propagater properly

Dagster version

dagster, version 1.9.2

Deployment type

Dagster Helm chart

Deployment details

No response

Additional information

No response

Message from the maintainers

This is a small bug, I can contribute a fix if the maintainers confirm this is indeed unintended behavior

@abhinavDhulipala abhinavDhulipala added the type: bug Something isn't working label Dec 19, 2024
@garethbrickman garethbrickman added the deployment: k8s Related to deploying Dagster to Kubernetes label Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment: k8s Related to deploying Dagster to Kubernetes type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants