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

support direct invocation with AssetExecutionContext #16635

Conversation

jamiedemaria
Copy link
Contributor

@jamiedemaria jamiedemaria commented Sep 19, 2023

Summary & Motivation

updates build_asset_context so that it creates a subclass of AssetExecutionContext

I'm not very familiar with this part of the code base. My general approach was to follow where UnboudOpExecutionContext and BoundOpExecutionContext are used and update them to also support Unbound and BoundAssetExecutionContext.

How I Tested These Changes

if isinstance(args[0], UnboundAssetExecutionContext):
context = cast(UnboundAssetExecutionContext, args[0])
else:
context = cast(UnboundOpExecutionContext, args[0])
Copy link
Member

Choose a reason for hiding this comment

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

isn't even worth doing all the casting at this point? Seems like the typechecker will be totally confused as to what type "context" is anyways.

From brief inspection it appears the only requirement of the context local variable is that it have a bind method, so it you really want that typed you can introduce an interface that has this, but this looks like mostly pointless, unnecessary gymnastics to appease typechecking gods, unless I am misreading this

@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from 5e7ed31 to dfd922e Compare September 20, 2023 16:43
@jamiedemaria jamiedemaria force-pushed the jamie/direct-invocation branch from 9c78c7e to 7c6b42a Compare September 20, 2023 16:43
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from dfd922e to 689e3f1 Compare September 20, 2023 17:08
@jamiedemaria jamiedemaria force-pushed the jamie/direct-invocation branch from 7c6b42a to 431ffab Compare September 20, 2023 17:08
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from 689e3f1 to 83a075e Compare September 20, 2023 17:36
@jamiedemaria jamiedemaria force-pushed the jamie/direct-invocation branch from 431ffab to 663d09f Compare September 20, 2023 17:36
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from 83a075e to ac9cb0b Compare September 20, 2023 18:23
@jamiedemaria jamiedemaria force-pushed the jamie/direct-invocation branch from 663d09f to 6183193 Compare September 20, 2023 18:23
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from ac9cb0b to 53a3b9a Compare September 20, 2023 21:17
@jamiedemaria jamiedemaria force-pushed the jamie/direct-invocation branch from 6183193 to ad48103 Compare September 20, 2023 21:17
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from 53a3b9a to 32d5c77 Compare September 20, 2023 21:24
@jamiedemaria jamiedemaria force-pushed the jamie/direct-invocation branch from ad48103 to cd94444 Compare September 20, 2023 21:24
@jamiedemaria jamiedemaria force-pushed the jamie/provide-asset-context branch from 32d5c77 to 5a064ff Compare September 20, 2023 22:06
@jamiedemaria jamiedemaria force-pushed the jamie/update-build-asset-context-usages branch from 7042434 to b29961c Compare September 22, 2023 15:16
@jamiedemaria jamiedemaria force-pushed the jamie/direct-invocation branch from 384d390 to 5c4494f Compare September 22, 2023 15:16
@jamiedemaria jamiedemaria force-pushed the jamie/update-build-asset-context-usages branch from b29961c to 5257cc4 Compare September 22, 2023 16:02
@jamiedemaria jamiedemaria force-pushed the jamie/direct-invocation branch from 5c4494f to 2c59e3b Compare September 22, 2023 16:02
@jamiedemaria jamiedemaria force-pushed the jamie/update-build-asset-context-usages branch from 5257cc4 to a53d70c Compare September 22, 2023 17:47
@jamiedemaria jamiedemaria force-pushed the jamie/direct-invocation branch from 2c59e3b to ab3472a Compare September 22, 2023 17:47
)


class UnboundOpExecutionContext(OpExecutionContext):
class BoundExecutionContext(OpExecutionContext):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having this interface be a child of OpExecutionContext feels shady, but it's the only way i was able to find to make the type checker happy.

When AssetExecutionContext eventually splits from OpExecutionContext there will be a good amount of work to do to make the path through op_invocation.py happy with the two types

@jamiedemaria jamiedemaria force-pushed the jamie/update-build-asset-context-usages branch from a53d70c to 5ca0129 Compare September 22, 2023 20:50
@jamiedemaria jamiedemaria force-pushed the jamie/direct-invocation branch from ab3472a to 2b23f4f Compare September 22, 2023 20:50
@jamiedemaria jamiedemaria force-pushed the jamie/update-build-asset-context-usages branch from 5ca0129 to 3b8faaa Compare September 26, 2023 14:06
@jamiedemaria jamiedemaria force-pushed the jamie/direct-invocation branch from 2b23f4f to db73b3c Compare September 26, 2023 14:06
@jamiedemaria jamiedemaria marked this pull request as draft October 13, 2023 19:18
@jamiedemaria
Copy link
Contributor Author

converting to draft to get out of review queues. will need to re-open or open a similar PR when method deprecations for Asset context start

@jamiedemaria
Copy link
Contributor Author

#18044 replaces this

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