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

replace direct calls to _step_execution_context with getter calls #25989

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pskinnerthyme
Copy link

Summary & Motivation

_step_execution_context does not exist on all subclasses (see DirectOpExecutionContext here ) and does not fail gracefully when attempting to get these properties.

Ran into this error when attempting to use dagster.build_op_context() during testing of a workflow which otherwise runs as a step. Received the following error:

self = <dagster._core.execution.context.invocation.DirectOpExecutionContext object at 0xffff51e5d850>

    @property
    def run_tags(self) -> Mapping[str, str]:
        """Mapping[str, str]: The tags for the current run."""
>       return self._step_execution_context.run_tags
E       AttributeError: 'DirectOpExecutionContext' object has no attribute '_step_execution_context'

but expected DagsterInvalidPropertyError

How I Tested These Changes

@jamiedemaria
Copy link
Contributor

hi @pskinnerthyme! sorry for the delayed response on this PR! I think this should be a fine code change to make, but i think you will need to pull in the latest changes from the dagster repo to get CI passing

@garethbrickman
Copy link
Contributor

garethbrickman commented Dec 16, 2024

@pskinnerthyme Could you please pull in the latest changes from master branch? Then we can re-run our CI tests

@pskinnerthyme pskinnerthyme marked this pull request as draft December 16, 2024 16:24
`_step_execution_context` does not exist on all subclasses (see `DirectOpExecutionContext`) and does not fail gracefully when attempting to get these properties.
@pskinnerthyme pskinnerthyme marked this pull request as ready for review December 16, 2024 16:31
@pskinnerthyme
Copy link
Author

Apologies for the delay -- updated. messed up my first pull, so had to fix that.

@garethbrickman garethbrickman removed the request for review from neverett December 16, 2024 16:33
@garethbrickman
Copy link
Contributor

garethbrickman commented Dec 16, 2024

@pskinnerthyme The test for ruff is failing- could you run make ruff in the root Dagster directory?

@garethbrickman
Copy link
Contributor

@pskinnerthyme After running make ruff you'd need to push any of the changes it did so the CI tests can pass

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants