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

Observable tasks deepdrills #446

Merged
merged 5 commits into from
Nov 30, 2021
Merged

Observable tasks deepdrills #446

merged 5 commits into from
Nov 30, 2021

Conversation

varunp2k
Copy link
Member

Added status messages for various steps in DeepDrills

@netlify
Copy link

netlify bot commented Nov 30, 2021

✔️ Deploy Preview for frontend-sb canceled.

🔨 Explore the source changes: af5f0c1

🔍 Inspect the deploy log: https://app.netlify.com/sites/frontend-sb/deploys/61a5dcf02a20ed00070bfcaa

Copy link
Contributor

@suranah suranah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should refrain from misspells or obsolete spells like computor for sub tasks. It’s okay for now as the sub tasks would be iterated with @pshrimal21

@suranah suranah merged commit 759d497 into main Nov 30, 2021
Copy link
Contributor

@Samyak2 Samyak2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@varunp2k @suranah @kartikay-bagla I had a few concerns

Comment on lines +314 to +315
self._checkpoint_failure("Data Loader", e)
self._checkpoint_success("Data Loader")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are specific to a timeline, right? We should add timeline name to the message as in aggregation and computation.

@@ -332,25 +339,35 @@ def compute(self):
try:
rca_data = self._get_rca(rca, dim, timeline)
output.append(self._output_to_row("rca", rca_data, timeline, dim))
except: # noqa E722
self._checkpoint_success("Base DeepDrills Calculator")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is specific to a dimension and a timeline. Should be added to the message, otherwise we'll see many instances of "Base DeepDrills calculator" repeated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think almost everything in RCA is timeline dependent

Comment on lines +360 to +362
self._checkpoint_failure("Htable Calculator", e)
# TODO: Checkpoint failure for htable calculation here.
self._checkpoint_success("Htable Calculator")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This too is specific to a timeline.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, it should be fine because the entries above it will have the timeline.

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.

None yet

4 participants