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

AIP-8718 Allow disabling default cards; Allow card render failures #309

Merged
merged 15 commits into from
Sep 30, 2024

Conversation

cloudw
Copy link
Collaborator

@cloudw cloudw commented Aug 23, 2024

Changes:

  • Default cards are still added by default to all steps, but user has option to disable it is by passing --add-default-cards false.
    • TODO: this option needs to be propagated to CICD
  • Card rendering errors are ignored.

Tests:

Closes AIP-8718

@cloudw cloudw self-assigned this Aug 23, 2024
@cavandervoort
Copy link

cavandervoort commented Aug 23, 2024

In general it looks good. I'm going to let Taleb decide to approve since he has experience with the repo.

I like the use of try/except. EDIT: I see you had in your slack message a note that you need to add tests.

metaflow/plugins/aip/aip_metaflow_step.py Outdated Show resolved Hide resolved
metaflow/plugins/aip/aip_metaflow_step.py Outdated Show resolved Hide resolved
metaflow/plugins/aip/aip.py Show resolved Hide resolved
metaflow/plugins/aip/aip_metaflow_step.py Outdated Show resolved Hide resolved
f"Failed to get cards from Metaflow backend for pathspec {pathspec}"
"Please view cards through Metaflow UI, or refer to https://docs.metaflow.org/metaflow/visualizing-results/effortless-task-inspection-with-default-cards#accessing-cards-via-an-api"
)
continue
Copy link
Collaborator

Choose a reason for hiding this comment

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

if we retry calling the metaflow metadata store, we should put a sleep in between retries, even a of 1s or 0.5 would make a difference.

metaflow/plugins/aip/aip_metaflow_step.py Outdated Show resolved Hide resolved
@cloudw cloudw merged commit ffc807a into feature/aip Sep 30, 2024
3 checks passed
@cloudw cloudw deleted the yunw/allow-card-disabling branch September 30, 2024 20:55
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.

3 participants