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

Add region property to DAG #677

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

spencerseale
Copy link
Collaborator

Add DAG.region for better detection of DAG host region

@spencerseale
Copy link
Collaborator Author

spencerseale commented Oct 31, 2024

The only issue I foresee is if someone overrides region after DAG already started. DAG.region as it's implemented here, gets current region. The alternative would be to return a non-NoneType only after DAG.compute is fired. Since a DAG is only usable for one task graph execution I see this alternative as more validation after running. However the original inspiration for this was to validate region prior to executing DAG.

@sgillies
Copy link
Collaborator

@spencerseale if I read the code correctly, the new property doesn't tell us for sure what region the DAG is running in, it tells us the current client configuration. Which is pretty much what you said above, right?

I think this goes against the OO principle of encapsulation. DAG methods should be based on DAG data, not data in another class or module.

@spencerseale
Copy link
Collaborator Author

@sgillies That's correct and you're right. I can take another stab at it so it's pinned to the DAG object.

@ihnorton ihnorton marked this pull request as draft November 4, 2024 18:39
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