Better handling for `undefined` compute configs

[dmfalke]

fixed #735

This PR adds some defensive code to handle undefined compute configs. It also introduces more specific types for compute configs (null | Record<string, unknown>).

• If a compute config is undefined, stub in the default using the plugin's createDefaultConfiguration function
• Require compute plugins to return either null or Record<string, unknown>, which better models our expectations.

[dmfalke]

Thanks @asizemore! I think the issue with XY is due to my changes. I made the config null in this PR. I will double check the viz code.

I'm not sure about the thumbnail issue. I'll double check that, as well.

[dmfalke]

For example, this analysis had a diff abund volcano plot that says it's not configured, but once i click on it i see that actually it is configured but the computation isnt started. To me that's fine and i wouldn't consider it broken!

It's hard to say, but I think this is due to some changes I made in #706. I'd say it's probably fine as-is.

[dmfalke]

Finally, while testing i noticed an issue with the XY Relationships scatterplot. I checked main and it exists there as well so it's not because of this branch! But it has to do with computeConfig being null so i wanted to mark it here in case some of the work is related.

We have some options:

1. Revert to using undefined
2. Convert null to undefined
3. Modify the service to allow null configs

2 is probably the easiest, but I'd like to consider 3. Would 3 require more than just a raml change?

[asizemore]

It's hard to say, but I think this is due to some changes I made in https://github.com/VEuPathDB/web-monorepo/pull/706. I'd say it's probably fine as-is.

Agree!!

... 2 is probably the easiest, but I'd like to consider 3. Would 3 require more than just a raml change?

Yep 3 would be just a raml change, so nothing too hard. The only downside i see with 3 is that the raml for the xy relationship scatterplot isn't reused elsewhere that do have computations. So it'd be adding an optional param that wouldn't get used. The scatterplots that do have a computation get their request structure from a different raml type.

Would 3 make life here a lot easier or make the code really nice? If it's a big win for the frontend we should go for it!

[dmfalke]

Would 3 make life here a lot easier or make the code really nice? If it's a big win for the frontend we should go for it!

I don't think it's really a big win :slightly_smiling_face:. I'm going to go back to undefined, and also add some code to convert null to undefined.

I'm reminded that the frontend doesn't really model these things the same as the backend. On the frontend, apps and computes are conflated, which I think is adding some confusion. I don't think there is much we can do about it, without requiring a big database migration.

[dmfalke]

@asizemore I pushed some more changes:

• Revert to using undefined for "compute apps" without configs
• Check if a compute config is needed in scatterplot and barplot. I think these are the only vizs that are used w/ and w/out computes.

I'd love it if you can test this with some existing vizs, but I understand if you are strapped for time. Thanks!

[asizemore]

awesome thanks @dmfalke ! I will do some testing this afternoon!

[dmfalke]

awesome thanks @dmfalke ! I will do some testing this afternoon!

Thank you, much appreciated!

[asizemore]

I only got through 2 analyses before my computer had a case of the mondays, but scatters and box plots looked good!

For sake of completeness, I saw that old compute vizs say "computation not configured" above, even though they just need to be run. As we discussed, that seems fine!

Good to go @dmfalke !

[dmfalke]

Awesome, thanks!