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

Update the error message and automatically download a dataset when a part of a recipe is not found #208

Merged
merged 49 commits into from
Jan 16, 2025

Conversation

sanjaycal
Copy link
Contributor

This change automatically downloads a dataset if a recipe requires one that is not downloaded, also this change contains commented code for model downloading that is not complete yet, but will be soon.

Copy link
Member

@dadmobile dadmobile left a comment

Choose a reason for hiding this comment

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

I'll have a closer look at this Monday morning. I think this is short-term either way. We should make a modal that shows you details and updates you on progress etc.

@sanjaycal
Copy link
Contributor Author

Yeah definitely

if (!response.dataset.downloaded) {
msg += "Download dataset " + response.dataset.path
msg += "\n Download dataset " + response.dataset.path + ", this will be done automatically";
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can automatically start these downloads without the user knowing what they are getting into? (even if we say this is just temporary)
My suggestion is:

  1. check if anything has to be downloaded. If so they show a confirm instead of an alert.
  2. If they click OK on the confirm THEN do the download.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be done now.

Copy link
Member

@dadmobile dadmobile left a comment

Choose a reason for hiding this comment

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

Good enough for now!

@dadmobile dadmobile merged commit e51b503 into main Jan 16, 2025
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