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

autoconversion: Fix/conversion filename bug #486

Merged
merged 161 commits into from
May 23, 2024
Merged

Conversation

interim17
Copy link
Contributor

@interim17 interim17 commented Mar 28, 2024

Time estimate or Size

Medium

This should be working with current viewer patch.

Problem

Converted trajectories were not downloadable, had no titles in the header, and were buggy when loaded after other trajectories.

Solution

The biggest part of this change is a new flow to receive converted files, first handleIncomingConvertedFile in the viewer panel and then receiveConvertedFileLogic in redux.

handleIncomingConvertedFile puts the name and title into state for use in the header, download menu, etc.

The receiveConvertedFileLogic calls controller.changeFile with the right arguments to keep the websocket connection we established during conversion, switching connections was causing bugs.

Some of this was fixed by recent viewer PR, and this change set brings the website into alignment with viewer in how it calls controller methods like convertTrajectory and changeFile.

Miscellaneous changes:

  • getIsNetworkedFile selector was checking if files are in TRAJECTORIES constant, but converted files won't always be there in auto-conversion era, removed check
  • in octopus era all downloads will point at new HTTP endpoint instead of s3, updated constant and added todo for launch, and adjusted download string to match in getHref
  • Bug fix (non-breaking change which fixes an issue)

To review:

  • convert a smoldyn file
  • when it arrives, check for title in status bar
  • download it with download button
  • load the downloaded trajectory
  • load a different trajectory and try the flow again

frasercl and others added 30 commits January 18, 2023 14:47
… into 346-implement-production-autoconversion-ui
Co-authored-by: Megan Riel-Mehan <meganrm@gmail.com>
… into 346-implement-production-autoconversion-ui
export const DATA_BUCKET_URL =
"https://aics-simularium-data.s3.us-east-2.amazonaws.com";
"https://staging-simularium-ecs.allencell.org:443/download";
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're handling download requests through octopus now, we should just use the BACKEND_SERVER_IP, like we do for defining the net connection settings, and remove this constant. Hard coding it to look at staging like we have here would break in production, because there won't be the same files in each for the case of autoconversion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, fixed this! We talked abou this, but I'm posting here for context for other reviewers that the .simularium file extension needed to be appended to the fileId earlier in the process to ensure that the download links for converted and other networked files also looked the same.

Copy link
Contributor

@ascibisz ascibisz left a comment

Choose a reason for hiding this comment

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

I may not be a redux expert, but this looks good to me!

@interim17 interim17 requested review from meganrm and ascibisz May 22, 2024 22:05
@@ -15,8 +15,7 @@ export const URL_PARAM_BASE_TYPES = "base_types.json";
export const URL_PARAM_CUSTOM_TYPES = "custom-types";
// URLs
export const BASE_API_URL = `/api/${API_VERSION}`;
export const DATA_BUCKET_URL =
"https://aics-simularium-data.s3.us-east-2.amazonaws.com";
export const DATA_BUCKET_URL = `https://${process.env.BACKEND_SERVER_IP}:443/download`;
Copy link
Contributor

@ascibisz ascibisz May 23, 2024

Choose a reason for hiding this comment

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

nit: this isn't really a bucket anymore, DOWNLOAD_URL may be more accurate

@interim17 interim17 merged commit 7182f54 into main May 23, 2024
6 checks passed
@interim17 interim17 deleted the fix/conversion-filename-bug branch May 23, 2024 19:25
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.

4 participants