Autoconversion Fixes #462

ascibisz · 2024-02-02T00:30:39Z

Problem

This is a somewhat large PR that solves three needs:

Need to send the file to Octopus to get it converted.
Receive the converted file from Octopus.
Front end errors arising from switching backends.

@interim17 did most of the work for this PR, @ascibisz helped with some debugging and happened to be the one to open the PR because she had the time to

Solution

Convert the file

Redux action/logic being triggered to send the file to Octopus. As always with redux there is much boiler plate, the action is in convertFileLogic which does the sending.

Receive the converted file

To receive the file we need a configured viewer at the ready, so Joe refactored the routes so that the conversion form is just a component that is just visible in front of the viewer when the user opens it. This made some of the communication with Octopus simpler. We're always at /viewer ready to receive a file, and we handle navigation in and out of the conversion form with conversionState.

Errors

Alli then did some miscellaneous debugging, as well as switching the autoconversion feature branch to use octopus for everything, not just the autoconversion point. The infrastructure team is actively working on the octopus deployment, so rather than continuing to have a weird state of partially using octopus and partially using simularium-engine, we think it makes sense to have this feature branch ready to fully embrace octopus.

interim17 · 2024-02-20T17:01:26Z

src/state/trajectory/logics.ts

+const convertFileLogic = createLogic({
+    process(
+        deps: ReduxLogicDeps,
+        dispatch: <T extends AnyAction>(action: T) => T,
+        done
+    ) {
+        const { getState } = deps;
+
+        const { engineType, fileToConvert, fileName } =
+            getConversionProcessingData(getState());
+        const fileContents: Record<string, any> = {
+            fileContents: { fileContents: fileToConvert },
+            metaData: { trajectoryTitle: fileName },
+        };
+        dispatch(
+            clearSimulariumFile({
+                newFile: true,
+            })
+        );
+        const controller = getSimulariumController(getState());
+        // convert the file
+        dispatch(
+            setConversionStatus({
+                status: CONVERSION_ACTIVE,
+            })
+        );
+        controller
+            .convertAndLoadTrajectory(
+                netConnectionSettings,
+                fileContents,
+                engineType
+            )
+            .catch((err: Error) => {
+                console.error(err);
+            });
+        done();
+    },
+    type: CONVERT_FILE,
+});
+


This is the logic that actually sends the file to Octopus for conversion. Because we are already at /viewer we are ready for Octopus to receive the converted file as soon as its ready.

interim17 · 2024-02-20T17:06:06Z

src/containers/Simularium/index.tsx

+                    {conversionStatus !== CONVERSION_INACTIVE && (
+                        <ConversionForm />
+                    )}


Now that the conversion process doesn't have its own path, we are rendering the conversion form here in Simularium because the viewer itself doesn't really need to know anything about conversion state, it just needs to handle the converted trajectory. Styling is also much easier here above the ViewerPanel layout.

…e/convert-file-no-routing

meganrm · 2024-03-11T17:11:23Z

src/containers/ConversionForm/custom-request.ts

    setError: (error: ViewerError) => void,
    rcRequest?: UploadRequestOption
 ) => {
    try {
        const unpackedFileText = await fileToConvert.originFileObj.text();
-        receiveFileToConvert(unpackedFileText);
+        const fileName = fileToConvert.name.replace(/\.[^/.]+$/, "");


why is this needed? can you add a comment about what this regex selects for. You can also always use a named variable for your regex, like const whiteSpace = RegEX; string.replace(whiteSpace, "")

meganrm · 2024-03-11T17:13:29Z

src/state/trajectory/logics.ts

+        dispatch(
+            clearSimulariumFile({
+                newFile: true,
+            })


it seems like you wouldn't want to clear it out until you've received the converted file. Because if they cancel it they should go back to the previous trajectory

Correct, just removing it seems to work well

frasercl

Only very small comments from me. I don't have the full perspective of how the app and octopus interact, but from what I understand this looks good!

frasercl · 2024-03-18T19:42:55Z

src/index.tsx

            batch(() => {
-                dispatch(setIsPlaying(false));
-                dispatch(setConversionStatus({ status: CONVERSION_NO_SERVER }));
-            });
-        } else if (location.pathname === VIEWER_PATHNAME) {
-            batch(() => {
-                dispatch(setIsPlaying(false));
                dispatch(setStatus({ status: VIEWER_EMPTY }));


nit: is this batch call still needed?

frasercl · 2024-03-18T19:55:57Z

src/containers/Simularium/index.tsx

@@ -77,6 +85,7 @@ class App extends React.Component<AppProps, AppState> {
    public simulariumController: SimulariumController | undefined;
    private interactiveContent = React.createRef<HTMLDivElement>();
    private endDragover = 0;
+    private conversionCheckInterval: NodeJS.Timeout | null = null;


Linter warns this is unused. Will it be used later?

Nope, needs removing

frasercl · 2024-03-18T20:07:30Z

src/containers/ConversionForm/index.tsx

 }: ConversionProps): JSX.Element => {
    const [fileToConvert, setFileToConvert] = useState<UploadFile | null>();
    const [engineSelected, setEngineSelected] = useState<boolean>(false);
+    const [isProcessing, setIsProcessing] = useState<boolean>(false);
    const [serverDownModalOpen, setServerIsDownModalOpen] =


nit (and not even one in the diff!): inconsistent names: serverDownModalOpen vs setServer*Is*DownModalOpen

Good catch, I think I now prefer serverErrorModalOpen anyway as it matches the modal component better

src/containers/ConversionForm/index.tsx

frasercl · 2024-03-18T20:25:11Z

src/containers/ConversionForm/index.tsx

@@ -70,21 +71,28 @@ const ConversionForm = ({
    receiveFileToConvert,
    initializeConversion,
    conversionStatus,
+    convertFile,
+    setConversionStatus,
 }: ConversionProps): JSX.Element => {
    const [fileToConvert, setFileToConvert] = useState<UploadFile | null>();
    const [engineSelected, setEngineSelected] = useState<boolean>(false);


Also not technically part of this PR (so feel free to ignore me!), but this looks like derived state to me. engineSelected tracks whether another component of state is set, and may become out of sync if not properly updated (e.g. engineSelected: false while store.trajectory.processingData.engineType: "cytosim"). This particular case is probably too simple to cause issues, but it still might be slightly better practice to select engineType out of the store and null-check it.

Love it, seems like this works and can remove the state variable:
const engineSelected = !!conversionProcessingData.engineType;

Co-authored-by: Cameron Fraser <53030214+frasercl@users.noreply.github.com>

frasercl

Looks good!

interim17 and others added 30 commits January 5, 2024 12:24

add convert file redux functions and constants

7b811ed

add convert file redux logic

d83fe6d

add ConversionProcessingData type

56a28c7

check and set conversionstatus in trajectory handler

41c00f5

set conversion file contents and file name

511fad8

cancel conversion via import path on overlay

f8b0d3f

render conversion processing overlay in sim container

a5583a5

conversion form will run convert file action and navigate to viewer

85a9c7a

set conversion status after any failed health check

1a23796

add health checks

70ed164

add import routing and interval health checks to sim container

dc525ab

some testing logs in redux logic

47dc500

remove import route

a0ff6ce

render conversion form in sim container

35722b1

render overlay in conversion form

129192b

update trajectory handler for new conversion status

f247119

handle conversion status in load file menu

452da2d

restore cancel processing as prop in conversion processing overlay

2bddf3c

style conversion form for new context

51e0236

remove light theme from overlay component

63c7654

consolidate octopus config for logics

3eceda3

clean up comments and imports

d2a58bc

provide onclick function for cancel conversion button

21aee37

add light theme text primary color

1aecd91

remove interval checks for server health

68e8637

update overlay style

633a96d

remove controller prop

d60eec3

no need to try to pause the simulation, it's abandoned at this point

69a3bcf

use octopus for autoconversion and for loading files

4580f02

remove unused imports

fb569f6

Merge branch 'fix/react-bump' into feature/convert-file-no-routing

0a24f6c

interim17 reviewed Feb 20, 2024

View reviewed changes

Base automatically changed from fix/react-bump to 346-implement-production-autoconversion-ui February 21, 2024 22:20

interim17 and others added 4 commits February 21, 2024 14:26

merge react bump changes

8bd7015

Merge branch '346-implement-production-autoconversion-ui' into featur…

9bcf6b2

…e/convert-file-no-routing

remove unused function

11eab97

remove unused import

8cf24c1

ascibisz requested a review from meganrm February 22, 2024 00:21

ascibisz marked this pull request as ready for review February 22, 2024 00:21

ascibisz requested review from toloudis and frasercl as code owners February 22, 2024 00:21

ascibisz removed the request for review from toloudis February 22, 2024 00:22

ascibisz changed the title ~~Misc Autoconversion Debugging~~ Autoconversion Fixes Feb 23, 2024

meganrm reviewed Mar 11, 2024

View reviewed changes

use const for file extension regex

3abe56b

frasercl reviewed Mar 18, 2024

View reviewed changes

interim17 and others added 7 commits March 20, 2024 10:22

Update src/containers/ConversionForm/index.tsx

e11d8c7

Co-authored-by: Cameron Fraser <53030214+frasercl@users.noreply.github.com>

don't clear out previous trajectory in convertFile logic

059a120

remove unused conversion check interavl property

698d0fd

rename server down modal

514d3e0

position conversion form on top of viewer

fca7d33

derive engineSelected from processingData state

51b0157

remove unneeded batch call

369a6f4

meganrm approved these changes Mar 20, 2024

View reviewed changes

interim17 requested a review from frasercl March 20, 2024 20:52

frasercl approved these changes Mar 22, 2024

View reviewed changes

interim17 merged commit efc3f61 into 346-implement-production-autoconversion-ui Mar 22, 2024
5 checks passed

interim17 deleted the feature/convert-file-no-routing branch March 22, 2024 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autoconversion Fixes #462

Autoconversion Fixes #462

ascibisz commented Feb 2, 2024 •

edited

Loading

interim17 Feb 20, 2024

interim17 Feb 20, 2024

meganrm Mar 11, 2024

meganrm Mar 11, 2024

interim17 Mar 20, 2024

frasercl left a comment

frasercl Mar 18, 2024

interim17 Mar 20, 2024

frasercl Mar 18, 2024

interim17 Mar 20, 2024

frasercl Mar 18, 2024

interim17 Mar 20, 2024

frasercl Mar 18, 2024

interim17 Mar 20, 2024

frasercl left a comment

Autoconversion Fixes #462

Autoconversion Fixes #462

Conversation

ascibisz commented Feb 2, 2024 • edited Loading

Problem

Solution

Convert the file

Receive the converted file

Errors

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frasercl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frasercl left a comment

Choose a reason for hiding this comment

ascibisz commented Feb 2, 2024 •

edited

Loading