[3/n] Allows user to disable snapshots from SDK #1400

likawind · 2023-06-01T23:30:30Z

Describe your changes and why you are making these changes

This PR adds SDK changes to disable snapshots on both artf and wf level. The API is implemented based on https://aqueducthq.slack.com/archives/C01U6RREC7M/p1685489357239879 . And the behavior looks like the following:

user can enable / disable snapshots each individual artifact (enabled by default)
when user turns off workflow level snapshot at publish_flow, this is equivalent to call disable_snapshots for all artifacts from non param / metrics / checks operators. In other words, if the user explicitly disable snapshots for those excluded artifacts, the option will be respected.
See the manual QA example as a reference to the above behavior

Related issue number (if any)

ENG-3000

Tests

Verified the manual QA as a part of #1401 .

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-3

likawind · 2023-06-01T23:32:35Z

manual_qa_tests/workflows/succeed_with_snapshots_disabled.py

+    return count < 10
+
+
+def deploy(client, resource_name):


@vsreekanti would like your input to confirm this call chain should result in:

reviews, op_artf and check_disabled_artf to be deleted

outputs of metrics and check are persisted

Actually I feel allowing disabling check artf is very confusing. I'm wondering if we should just return an error if the user attempts to do so. Maybe the same applies to params.

Disabling metric artf makes sense to me though, since esp. table metrics can have sensitive content as they are just a grammar sugar over regular operators.

cw75 · 2023-06-02T20:37:54Z

sdk/aqueduct/client.py

+            disable_snapshots:
+                If set 'True', disables snapshots for all artifacts not generating from params / metrics / checks
+                by calling `.disable_snapshot()` on each of these artifacts.


This sentence seems fragmented and I am a bit confused about what it means. Pasting it to GPT gave me the following:

disable_snapshots: If set to 'True', this option disables snapshots for all artifacts that are not generated from parameters, metrics, or checks. It achieves this by calling the `.disable_snapshot()` method on each of these artifacts.

Is this what you're trying to say? @likawind

Great, thank you :)

kenxu95

Can you paste a loom demo of the manual_qa_tests?

kenxu95 · 2023-06-05T21:00:34Z

sdk/aqueduct/artifacts/base_artifact.py

+    def enable_snapshot(self) -> None:
+        self._dag.update_artifact_should_persist(self._artifact_id, True)
+
+    def disable_snapshot(self) -> None:


Can we use the set_ prefix for any write methods?

It's been discussed in https://aqueducthq.slack.com/archives/C01U6RREC7M/p1685489357239879 and given it's this close to release, I'd stick with the decision than making more back-and-forth to the channel. We've been using other verbs like bound max over artifacts so I don't see it as a huge misalignment as long as they start with verbs

oh I didn't realize this was user-facing actually. That's good for now yea

kenxu95 · 2023-06-05T21:02:45Z

sdk/aqueduct/models/dag.py

+    def update_artifact_should_persist(self, artifact_id: uuid.UUID, should_persist: bool) -> None:
+        self.must_get_artifact(artifact_id).should_persist = should_persist
+
+    def disable_snapshots_from_non_params_metrics_checks(self) -> None:


I personally find this name a little confusing. I'd prefer if we kept the method more generic disable_snapshots() and added a nice docstring here indicating that parameters, metric and check artifacts are excluded.

I actually prefer the naming to be self-descriptive esp. that here the underlying assumption is not clear to whoever scan through the caller codes.

how about disable_snapshots_not_from_params_metrics_checks ?

I also prefer my methods to be meaningful, but the description needs to stop somewhere. In this case, it feels like we're leaking implementation details into the method name. Specifically, I'd rather not be listing things in my names (params, metrics, checks), since it's not scalable or very readable imo (also not consistent with rest of code). Nbd if you want to keep it, it just jumped out at me.

I'm more worried that disable_snapshot is misleading because we don't actually disable all snapshots. I do agree that this name is too long and what you mentioned about listing stuff makes sense. Welcome to advices as long as we somehow indicate only some snapshots get disabled here :)

I've been thinking about things like disable_user_snapshots but this is also inaccurate..

disable_user_data_snapshots?

Updated to disable_sensitive_snapshots. sensitive may confuse reader but that should lead them to the comment where we clarify things

mm does disable_op_snapshots or disable_op_output_snapshots work? No strong opinions as long as we document it in the function.

yeah sounds good, I guess I just don't want it look too harmless so that caller reader would try to look at the docs, rather than skip over it with some wrong assumptions

kenxu95 · 2023-06-05T21:03:22Z

sdk/aqueduct/models/artifact.py

@@ -14,6 +14,7 @@ class ArtifactMetadata(BaseModel):
    # If true, this artifact name is expected to be unique in the DAG.
    explicitly_named: bool = False
    from_local_data: bool = False
+    should_persist: bool = True


Add a comment here about what should_persist means.

likawind · 2023-06-06T00:01:19Z

@kenxu95 manual QA is updated in #1401

likawind added 2 commits June 1, 2023 15:35

Working

f335cfa

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

b1a6102

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

likawind requested review from kenxu95, cw75 and vsreekanti June 1, 2023 23:30

likawind commented Jun 1, 2023

View reviewed changes

likawind added 2 commits June 1, 2023 16:37

comments

9dd9075

Add deleted to exec state

08d857a

cw75 reviewed Jun 2, 2023

View reviewed changes

Fix tests

6fe973e

kenxu95 reviewed Jun 5, 2023

View reviewed changes

comments

19a1500

kenxu95 self-requested a review June 6, 2023 00:12

kenxu95 approved these changes Jun 6, 2023

View reviewed changes

naming

0be87fb

cw75 approved these changes Jun 6, 2023

View reviewed changes

[4/n] Add UI changes to render deleted artifacts (#1401)

552bddb

likawind merged commit f557240 into eng-3000-allow-users-to-opt-out-of-data-snapshots-2 Jun 6, 2023
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3/n] Allows user to disable snapshots from SDK #1400

[3/n] Allows user to disable snapshots from SDK #1400

likawind commented Jun 1, 2023 •

edited

Loading

likawind Jun 1, 2023

likawind Jun 1, 2023

cw75 Jun 2, 2023

likawind Jun 3, 2023

kenxu95 left a comment

kenxu95 Jun 5, 2023

likawind Jun 5, 2023

kenxu95 Jun 6, 2023

kenxu95 Jun 5, 2023

likawind Jun 5, 2023

likawind Jun 5, 2023

kenxu95 Jun 5, 2023 •

edited

Loading

likawind Jun 5, 2023 •

edited

Loading

likawind Jun 5, 2023 •

edited

Loading

likawind Jun 5, 2023

kenxu95 Jun 6, 2023

likawind Jun 6, 2023 •

edited

Loading

kenxu95 Jun 5, 2023

likawind commented Jun 6, 2023

[3/n] Allows user to disable snapshots from SDK #1400

[3/n] Allows user to disable snapshots from SDK #1400

Conversation

likawind commented Jun 1, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenxu95 Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

likawind Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

likawind Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

likawind Jun 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

likawind commented Jun 6, 2023

likawind commented Jun 1, 2023 •

edited

Loading

kenxu95 Jun 5, 2023 •

edited

Loading

likawind Jun 5, 2023 •

edited

Loading

likawind Jun 5, 2023 •

edited

Loading

likawind Jun 6, 2023 •

edited

Loading