-
Notifications
You must be signed in to change notification settings - Fork 293
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 support for mapping over remote launch plans #2761
Conversation
Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this. Let's try to keep the interface clean by not using Any
at this point, unless we have a very good reason.
flytekit/core/array_node.py
Outdated
@@ -27,7 +31,7 @@ | |||
class ArrayNode: | |||
def __init__( | |||
self, | |||
target: LaunchPlan, | |||
target: Any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use Union
to describe the types? I'd rather keep ArrayNode
well-typed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup - getting an update for this. Was working around a circular imports error. Can use if TYPE_CHECKING
Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
I think there's some circular dependency that needs to happen but I think maybe what's here right now is more than necessary. let's chat about this some time today or tomorrow? |
Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
* add support for mapping over remote launch plans Signed-off-by: Paul Dittamo <pvdittamo@gmail.com> * add unit tests Signed-off-by: Paul Dittamo <pvdittamo@gmail.com> * lint Signed-off-by: Paul Dittamo <pvdittamo@gmail.com> * update var name Signed-off-by: Paul Dittamo <pvdittamo@gmail.com> * utilize TYPE_CHECKING to check for FlyteLaunchPlan Signed-off-by: Paul Dittamo <pvdittamo@gmail.com> * utilize adding remote node function Signed-off-by: Paul Dittamo <pvdittamo@gmail.com> * add unit tests Signed-off-by: Paul Dittamo <pvdittamo@gmail.com> * revert changes to local execute Signed-off-by: Paul Dittamo <pvdittamo@gmail.com> --------- Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
* add support for mapping over remote launch plans Signed-off-by: Paul Dittamo <pvdittamo@gmail.com> * add unit tests Signed-off-by: Paul Dittamo <pvdittamo@gmail.com> * lint Signed-off-by: Paul Dittamo <pvdittamo@gmail.com> * update var name Signed-off-by: Paul Dittamo <pvdittamo@gmail.com> * utilize TYPE_CHECKING to check for FlyteLaunchPlan Signed-off-by: Paul Dittamo <pvdittamo@gmail.com> * utilize adding remote node function Signed-off-by: Paul Dittamo <pvdittamo@gmail.com> * add unit tests Signed-off-by: Paul Dittamo <pvdittamo@gmail.com> * revert changes to local execute Signed-off-by: Paul Dittamo <pvdittamo@gmail.com> --------- Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
Tracking issue
fixes: https://linear.app/unionai/issue/COR-1686/support-mapping-over-flytelaunchplan-remote
Why are the changes needed?
Mapping over remote launch plans doesn't work
What changes were proposed in this pull request?
How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link