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

split up context/compute.py into multiple modules #23171

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

sryza
Copy link
Contributor

@sryza sryza commented Jul 23, 2024

Summary & Motivation

I sometimes get lost in compute.py and find it hard to tell which context class I'm in. I think this will help.

How I Tested These Changes

@sryza sryza requested a review from jamiedemaria July 23, 2024 15:52
branch-name: asset-execution-context
from contextlib import contextmanager
from contextvars import ContextVar
from inspect import _empty as EmptyAnnotation
from typing import AbstractSet, Any, Dict, Iterator, List, Mapping, Optional, Sequence, Union, cast
from typing import Iterator, Optional, Union
Copy link
Contributor

Choose a reason for hiding this comment

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

i always found the name of this file confusing - maybe now that its just the context manager maybe it could be like enter_context_helper or something else. not beholden to this though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way I had interpreted it was that "compute context" is the more general term that spans OpExecutionContext, AssetExecutionContext, and AssetCheckExecutionContext. I don't feel particularly invested in preserving this, but I do think enter_context_helper would be a little narrow, given that this module also holds ExecutionContextTypes and current_execution_context.

In light of that, going to punt figuring out the right name to a future PR.

@sryza sryza merged commit 2bb3411 into master Jul 24, 2024
1 check failed
@sryza sryza deleted the asset-execution-context branch July 24, 2024 20:06
sryza added a commit that referenced this pull request Jul 24, 2024
## Summary & Motivation

I sometimes get lost in compute.py and find it hard to tell which
context class I'm in. I think this will help.

## How I Tested These Changes
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.

2 participants