[5/n] Adds integration test for disabling snapshots #1404

likawind · 2023-06-03T00:43:47Z

Describe your changes and why you are making these changes

This PR adds integration tests for disabling snapshots. To properly examine the exec states of flows, we also added exec state tracking for all artifact types.

Also included a fix for ENG-3103 to fix datetime parsing

Related issue number (if any)

ENG-3000 ENG-3103

Tests

Integration tests on this PR should succeed, implying new feature working and no regressions.
Manual UI testing covered in previous PR [4/n] Add UI changes to render deleted artifacts #1401
Tested on a cleaner build to verify requirements.txt are updated (install only using pip install . and install_local script, without install from a pypi package).

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)

…to eng-3000-allow-users-to-opt-out-of-data-snapshots-5

integration_tests/sdk/aqueduct_tests/flow_test.py

kenxu95 · 2023-06-06T00:17:54Z

sdk/aqueduct/artifacts/generic_artifact.py

-            if self._execution_status != ExecutionStatus.SUCCEEDED:
+            if (
+                not self._execution_state
+                or self._execution_state.status != ExecutionStatus.SUCCEEDED


Um shouldn't this return None if the artifact was deleted? The error message talks about "computed successfully!" which is true when status is Deleted.

That's a good catch! It's interesting that we are not handling other state though. Following the same argument I think we should. I will just address the DELETE state in this PR though

What do you mean by handling other state? None of the other states computed the artifact successfully.

I mean for fetched workflow runs, it's reasonable to call .get() even if the artifact is with fail / cancel / pending state with less blocking behavior like throwing this error

yeah I think we just wanted to disambiguate a failed artifact vs an operator that returned None.

I see, in that way I will add a few lines to print warning message if the artifact is deleted

kenxu95 · 2023-06-06T00:19:19Z

sdk/aqueduct/backend/api_client.py

@@ -669,10 +668,10 @@ def get_artifact_result_data(
        if "data" in parsed_response:
            return_value = deserialize(serialization_type, artifact_type, parsed_response["data"])

-        if execution_status != ExecutionStatus.SUCCEEDED:
+        if not execution_state or execution_state.status != ExecutionStatus.SUCCEEDED:


Same question here about handling of DELETED status. Can we get away with just returning return_value in that case (if it's None?)

@likawind was this addressed? I'm not sure it's clear to me what happens when you call this on a deleted artifact. It would be great if that was in the function's docstring too

oh oh this line just prints a warning and doesn't affects the execution flow

oh I missed that lol. Can you note that DELETED -> None in the docstring though.

yeah updated the warning message and docstring

likawind · 2023-06-06T01:15:37Z

@kenxu95 thanks for the feedbacks! updated

kenxu95

It's good, I just had one blocking comment about get_artifact_result_data()

sdk/aqueduct/artifacts/generic_artifact.py

kenxu95 · 2023-06-06T02:43:01Z

sdk/aqueduct/artifacts/table_artifact.py

@@ -128,6 +129,9 @@ def head(self, n: int = 5, parameters: Optional[Dict[str, Any]] = None) -> pd.Da
            A dataframe containing the table contents of this artifact.
        """
        df = self.get(parameters=parameters)
+        if df is None:
+            return None


nit: slightly more python for `return df.head(n) if df else None?

oh actually I do this intentionally as I believe if df will be False if df is empty. But we can shrink that to one line though

kenxu95 · 2023-06-06T02:44:22Z

sdk/aqueduct/backend/api_client.py

@@ -669,10 +668,10 @@ def get_artifact_result_data(
        if "data" in parsed_response:
            return_value = deserialize(serialization_type, artifact_type, parsed_response["data"])

-        if execution_status != ExecutionStatus.SUCCEEDED:
+        if not execution_state or execution_state.status != ExecutionStatus.SUCCEEDED:


@likawind was this addressed? I'm not sure it's clear to me what happens when you call this on a deleted artifact. It would be great if that was in the function's docstring too

kenxu95 · 2023-06-06T02:54:47Z

sdk/aqueduct/backend/api_client.py

@@ -669,10 +668,10 @@ def get_artifact_result_data(
        if "data" in parsed_response:
            return_value = deserialize(serialization_type, artifact_type, parsed_response["data"])

-        if execution_status != ExecutionStatus.SUCCEEDED:
+        if not execution_state or execution_state.status != ExecutionStatus.SUCCEEDED:


oh I missed that lol. Can you note that DELETED -> None in the docstring though.

cw75

LGTM!

works

fee26f4

likawind added the run_integration_test Triggers integration tests label Jun 3, 2023

likawind added 3 commits June 2, 2023 17:49

Merge branch 'eng-3000-allow-users-to-opt-out-of-data-snapshots-4' in…

1adbc69

…to eng-3000-allow-users-to-opt-out-of-data-snapshots-5

Merge branch 'eng-3000-allow-users-to-opt-out-of-data-snapshots-4' in…

e2917ee

…to eng-3000-allow-users-to-opt-out-of-data-snapshots-5

Merge branch 'eng-3000-allow-users-to-opt-out-of-data-snapshots-4' in…

63a678f

…to eng-3000-allow-users-to-opt-out-of-data-snapshots-5

kenxu95 reviewed Jun 6, 2023

View reviewed changes

comments

d0464bd

kenxu95 reviewed Jun 6, 2023

View reviewed changes

kenxu95 approved these changes Jun 6, 2023

View reviewed changes

messages

18bebd0

cw75 approved these changes Jun 6, 2023

View reviewed changes

more comments

933fda9

likawind merged commit d503533 into eng-3000-allow-users-to-opt-out-of-data-snapshots-4 Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[5/n] Adds integration test for disabling snapshots #1404

[5/n] Adds integration test for disabling snapshots #1404

likawind commented Jun 3, 2023 •

edited

Loading

kenxu95 Jun 6, 2023

likawind Jun 6, 2023

kenxu95 Jun 6, 2023

likawind Jun 6, 2023

kenxu95 Jun 6, 2023

likawind Jun 6, 2023

kenxu95 Jun 6, 2023

kenxu95 Jun 6, 2023

likawind Jun 6, 2023

kenxu95 Jun 6, 2023

likawind Jun 6, 2023

likawind commented Jun 6, 2023

kenxu95 left a comment

kenxu95 Jun 6, 2023

likawind Jun 6, 2023

kenxu95 Jun 6, 2023

kenxu95 Jun 6, 2023

cw75 left a comment

[5/n] Adds integration test for disabling snapshots #1404

[5/n] Adds integration test for disabling snapshots #1404

Conversation

likawind commented Jun 3, 2023 • edited Loading

Describe your changes and why you are making these changes

Related issue number (if any)

Tests

Checklist before requesting a review

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

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 commented Jun 6, 2023

kenxu95 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

cw75 left a comment

Choose a reason for hiding this comment

likawind commented Jun 3, 2023 •

edited

Loading