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

Support uploading model runs from the web client #490

Merged
merged 20 commits into from
Sep 13, 2024

Conversation

floryst
Copy link
Contributor

@floryst floryst commented Aug 15, 2024

Add support for uploading model runs from the web client.

This draft PR gets the end-to-end pipeline of uploading and processing a zip file of geojsons. There are a couple of extra things to do to complete this PR.

  • Cleanup the UI
  • Enforce certain validations on the UI (e.g. not allowed to submit without a title)
  • Send the private flag, and apply the private flag on the backend
  • Run code linter
  • Address the TODOs

The core pipeline can use an initial review pass.

Screenshots:
image

image image

@floryst floryst force-pushed the client-upload-model-runs branch 5 times, most recently from 3cb8427 to 092e1ff Compare August 28, 2024 19:18
@floryst
Copy link
Contributor Author

floryst commented Aug 28, 2024

@BryonLewis is there an existing notion of a private/public model run in RD-WATCH?

Bryon: No there isn't. I've added fields to the Region and model-run endpoints to indicate if a item is public (it defaults to true). I haven't done any filtering or sorting on those fields yet (except maybe custom regions).

@floryst floryst force-pushed the client-upload-model-runs branch 4 times, most recently from e6eb50b to b9475c8 Compare August 29, 2024 18:44
@floryst floryst marked this pull request as ready for review August 29, 2024 18:44
@floryst floryst force-pushed the client-upload-model-runs branch 5 times, most recently from 97bc05b to 91de8e0 Compare September 3, 2024 18:55
Copy link
Contributor

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

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

I haven't tested yet, just looked at the code and added a few suggestions.

@@ -719,3 +724,99 @@ def download_sam_model_if_not_exists(**kwargs):
return f'Error downloading file: {e}'
else:
return f'File already exists at {file_path}'

Copy link
Contributor

Choose a reason for hiding this comment

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

After going through the animation export I think we should probably move some of these into their own file. In the future we should break up the __init__.py at least into an image downloading file (there are a lot of functions that handle that.).

vue/src/components/SideBar.vue Outdated Show resolved Hide resolved
vue/src/components/UploadModelRun.vue Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@floryst
Copy link
Contributor Author

floryst commented Sep 4, 2024

Recent updates:

  • use django-s3-file-field with s3 in prod, minio in dev
  • properly type the VFileInput model value
  • skip originator validation in SiteFeature. This is necessary because ingested geojsons may have originators that are not yet in the database. We can safely skip this check because we will add the missing performers later before saving the model run.
    • side note: the implementation would be better with pydantic v2 contexts
  • refresh the performers and regions when a successful upload occurs

Copy link
Contributor

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

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

Sorry I've been a bit busy on other projects but got around to review this. Some of the comments are generic things that could be implemented in the future. The main things are:

  • The Region override not working properly
  • I think it would be good to get the modelrun list to refresh on upload task completion.

The section about allowing multiple uploads to be processed at once and having the dialog be non-blocking isn't as important now considering modelruns should be processed in a few seconds.

rdwatch/core/tasks/__init__.py Outdated Show resolved Hide resolved
rdwatch/core/tasks/__init__.py Outdated Show resolved Hide resolved
vue/src/components/SideBar.vue Outdated Show resolved Hide resolved
vue/src/components/SideBar.vue Outdated Show resolved Hide resolved
vue/src/components/UploadModelRun.vue Show resolved Hide resolved
</v-card-actions>
</v-card>
</v-dialog>
<v-dialog v-model="successDialog" persistent width="25%">
Copy link
Contributor

Choose a reason for hiding this comment

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

In regards to this being persistent.

Not a huge issue because I know the infrastructure to properly handle it would be annoying. I'm wondering if something that is dependent on a celery worker being in an available state and running the task should block access to the rest of the application while it's processing.

When I'm talking about infrastructure I mean that if we have userId tied to the upload and the task we could add another endpoint in the views model_run for listing all currently active upload tasks for a user. The user could then see the progress of those tasks. To know they are currently doing something. This gives the extra benefit of not needing to block user input on fields while an upload is processing. Allowing for a user to upload a zip file and while it is processing upload another zip file with no delay. These are just thoughts right now and probably don't need to be implemented for this PR.

rdwatch/core/views/model_run.py Outdated Show resolved Hide resolved
@floryst floryst changed the base branch from main to fix-modelrunlist-styles-and-pagination September 12, 2024 15:51
@floryst
Copy link
Contributor Author

floryst commented Sep 12, 2024

This PR now depends on #502 and #503.

Base automatically changed from fix-modelrunlist-styles-and-pagination to main September 12, 2024 20:19
rdwatch/core/tasks/__init__.py Outdated Show resolved Hide resolved
rdwatch/core/tasks/__init__.py Outdated Show resolved Hide resolved
@BryonLewis BryonLewis self-requested a review September 13, 2024 19:37
Copy link
Contributor

@BryonLewis BryonLewis left a comment

Choose a reason for hiding this comment

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

Tested on a full unique zip with custom regions and size and it worked perfectly. Even the overrides worked well.

@floryst floryst merged commit 0d346cb into main Sep 13, 2024
8 checks passed
@floryst floryst deleted the client-upload-model-runs branch September 13, 2024 20:01
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