[SDK] Fail when using any integrations that aren't connected yet #1223

kenxu95 · 2023-04-18T15:08:45Z

@likawind and @hsubbaraj-spiral as lead reviewers.

Describe your changes and why you are making these changes

A couple SDK improvements in this:

The integration.describe() method now shows the status and the error if the integration failed to connect:

Methods on integrations that are decorated with @validate_is_connected() will not be allowed to be executed if the integration is not properly connected:

The same thing applies for when we set the engine of an operator to an invalid integration.

Related issue number (if any)

Loom demo (if any)

Checklist before requesting a review

I have created a descriptive PR title. The PR title should complete the sentence "This PR...".
I have performed a self-review of my code.
I have included a small demo of the changes. For the UI, this would be a screenshot or a Loom video.
If this is a new feature, I have added unit tests and integration tests.
I have run the integration tests locally and they are passing.
I have run the linter script locally (See python3 scripts/run_linters.py -h for usage).
All features on the UI continue to work correctly.
Added one of the following CI labels:
- run_integration_test: Runs integration tests
- skip_integration_test: Skips integration tests (Should be used when changes are ONLY documentation/UI)

kenxu95 · 2023-04-18T15:09:17Z

sdk/aqueduct/artifacts/preview.py

-        global_engine_config = generate_engine_config(
-            globals.__GLOBAL_API_CLIENT__.list_integrations(),
-            globals.__GLOBAL_CONFIG__.engine,
-        )


non-functional change. It just looks better to have the two subgraph statements next to each other.

likawind

Overall is good, left one suggestion to improve the ownership of validate_is_connected decorator

likawind · 2023-04-19T22:38:43Z

sdk/aqueduct/integrations/validation.py

+AnyFunc = Callable[..., Any]
+
+
+def validate_is_connected() -> Callable[[AnyFunc], AnyFunc]:


I'd prefer exposing this as a method in integration base class and have all methods use @self.validate_is_connected(). In this way the method is tied to the integration and it's clear that we are ensuring 'this' integration is connected. It also avoids passing around self to an externally imported method (I'm not super sure if it's an antipattern but it feels weird to read :) )

hmm I don't think I can use the @self.validate_is_connected() syntax.. My intellij errors and also https://stackoverflow.com/questions/11731136/class-method-decorator-with-self-arguments#:~:text=You%20can't.,use%20a%20different%20method%20entirely.

There's also a bit of a layering issue, where I'd have to move the validate_integration_is_connected() down to the models/ layer from utils/, which I feel doesn't make too much sense. This is all because the integration classes are defined in integrations/ and the base integration class is defined at a much lower level in models. I think these layering issues in the SDK should be addressed, but probably not in this PR.

What do you think?

yeah the current way looks fine then!

likawind · 2023-04-19T22:45:40Z

src/golang/lib/lambda/utils.go

@@ -78,7 +78,11 @@ func CreateLambdaFunction(ctx context.Context, functionsToShip []LambdaFunctionT
 	defer close(pushImageChannel)
 	AddFunctionTypeToChannel(functionsToShip[:], pullImageChannel)

-	for i := 0; i < MaxConcurrentDownload; i++ {
+	numPullWorkers := MaxConcurrentDownload


what are these for?

We pull the lambda Docker images from Dockerhub in parallel, this is making sure that the number of workers isn't greater than necessary i believe. @kenxu95 can confirm here?

yeah that's what it's doing. It's just leftover from when I was trying out the lambda stuff, but seemed useful.

likawind

Sry, mean't to block until we have clarity on the decorator :)

hsubbaraj-spiral · 2023-04-20T17:17:21Z

sdk/aqueduct/models/integration.py

+                    self._metadata.exec_state.error.context,
+                )
+            )
+        else:


Is there an explicit ExecutionStatus.Pending status to check for in progress, or is it just that after 2813, all statuses that are None are considered in progress?

Nah I don't think this has to do with ENG-2813 - that's just saying that the None case will no longer exist. I don't think we ever have a pending state, we just go directly to running. Can make that a harder assertion here.

Actually, I decided to simply document this assumption. The assertion seems too aggressive since this isn't a code path we are really testing right now.

sounds good

some fixes shoot. Maybe it's better to keep it two tables moved it to an integration_connection table, because of editing the migration works. Now to worry about the read and write paths added the drop column one ok now I'm confused. Unwinding back to a single table approach. there are so many thing swrong with this but at least I see a path through now. made some progress. Write path internals in a good state unwound a bunch of stuff prune all the stuff that is unnecessary now Moved the transaction back inside reverted a bunch of stuff back, still need too drop validated column added validated column drop. Ready for testing now. remove dsdk changes last things, lint and block usage of broken integrations. lint done small formatting fixes fixed gh actions made exec_state optional on SDK cr fixed err removed elem context switch made the change for all data integrations done? it's ready. Gonna test update lambda linted and rebased golang lint

kenxu95 · 2023-04-20T18:28:00Z

@likawind re-requesting review to sync on a path forward for the decorator.

kenxu95 requested review from cw75, likawind and hsubbaraj-spiral April 18, 2023 15:08

kenxu95 commented Apr 18, 2023

View reviewed changes

kenxu95 force-pushed the eng-2782-track-the-status-of-a-resource branch from bbd06a7 to b4d3694 Compare April 18, 2023 15:41

kenxu95 force-pushed the eng-2783-fail-when-using-any-resource-that-is-not branch 2 times, most recently from c6b91b4 to bc687b1 Compare April 18, 2023 20:55

kenxu95 force-pushed the eng-2782-track-the-status-of-a-resource branch from f516f10 to fb1047e Compare April 19, 2023 15:33

kenxu95 force-pushed the eng-2783-fail-when-using-any-resource-that-is-not branch from bc687b1 to 49fe047 Compare April 19, 2023 15:48

likawind approved these changes Apr 19, 2023

View reviewed changes

likawind requested changes Apr 19, 2023

View reviewed changes

hsubbaraj-spiral reviewed Apr 20, 2023

View reviewed changes

Base automatically changed from eng-2782-track-the-status-of-a-resource to main April 20, 2023 17:30

kenxu95 force-pushed the eng-2783-fail-when-using-any-resource-that-is-not branch from c9001d7 to 9b0a8d1 Compare April 20, 2023 18:13

lint and small comment change

73b29d9

kenxu95 requested a review from likawind April 20, 2023 18:27

likawind approved these changes Apr 20, 2023

View reviewed changes

kenxu95 requested a review from hsubbaraj-spiral April 20, 2023 19:47

kenxu95 added the run_integration_test Triggers integration tests label Apr 20, 2023

done

db1e3ae

hsubbaraj-spiral approved these changes Apr 20, 2023

View reviewed changes

kenxu95 added 2 commits April 20, 2023 19:16

fixed test

5f72a2a

ok now tests are fixed

14f12cf

kenxu95 merged commit a223adf into main Apr 21, 2023

kenxu95 deleted the eng-2783-fail-when-using-any-resource-that-is-not branch April 21, 2023 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SDK] Fail when using any integrations that aren't connected yet #1223

[SDK] Fail when using any integrations that aren't connected yet #1223

kenxu95 commented Apr 18, 2023

kenxu95 Apr 18, 2023

likawind left a comment

likawind Apr 19, 2023

kenxu95 Apr 20, 2023

likawind Apr 20, 2023

likawind Apr 19, 2023

hsubbaraj-spiral Apr 20, 2023

kenxu95 Apr 20, 2023

likawind left a comment

hsubbaraj-spiral Apr 20, 2023

kenxu95 Apr 20, 2023

kenxu95 Apr 20, 2023

hsubbaraj-spiral Apr 20, 2023

kenxu95 commented Apr 20, 2023

		AnyFunc = Callable[..., Any]


		def validate_is_connected() -> Callable[[AnyFunc], AnyFunc]:

[SDK] Fail when using any integrations that aren't connected yet #1223

[SDK] Fail when using any integrations that aren't connected yet #1223

Conversation

kenxu95 commented Apr 18, 2023

Describe your changes and why you are making these changes

Related issue number (if any)

Loom demo (if any)

Checklist before requesting a review

Choose a reason for hiding this comment

likawind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

likawind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenxu95 commented Apr 20, 2023