Allow inlined root sequence #1688
Conversation
nextstrain-bot
temporarily deployed
to
auspice-inline-root-seq-d0lglm
There was a problem hiding this comment.
Looked at the code, looks fine, didn't try the functionality. One question about promise handling just so I understand better, but its non-blocking. I also noticed that the commits omit any reason why this inlining is now being supported/allowed. That would be nice for posterity.
src/actions/loadData.js
Outdated
| * The returned promise is used by `loadSidecars` which will append `.then` clauses to the promise; | ||
| * often the load function is called immediately (and so the promises are unresolved) but for | ||
| * narratives it may be called some time later. Because of this the promises should not reject - | ||
| * if there is a fetch error then we resolve to the error and it is the responsibility of | ||
| * `loadSidecars` to react appropriately. |
There was a problem hiding this comment.
non-blocking
It's not clear to me from anything in this commit why the rejected promises have to be converted to promises that resolve to an Error. Why can't the rejection be chained thru? It's possible to chain a .catch() on a rejected promise well after it's already been fulfilled.
Hmm… Is the sole purpose here to avoid the browser's default behaviour of emitting a console error message for an "unhandled rejection" (incorrectly/prematurely sometimes, though it doesn't know that). That default rejection event behaviour is changeable.
There was a problem hiding this comment.
My understanding is that you don't have control over unhandled rejections on a per-promise level. In general, I do want unhandled rejections to result in error logs as most of the time they are coding errors, but not in this case. I'll update the message to indicate this is why the promises should not reject.
There was a problem hiding this comment.
Yeah, not easily on a per-promise level; you do always have to interact in some way with the global events/handlers.
Definitely want to surface unhandled rejections somehow for the reason that they're a programming error, yes, though I think the default global events/handlers are a very minimal, not very good example of that surfacing. As a example of an alternative, I'd consider the approach I used here as an improvement:
This example is from Node.js and uses process exit as the trigger for reporting as-of-yet-unhandled rejections. In a browser context, you could use other triggers (likely more frequent), such as "finished loading" events specific to the app (e.g. a periodic barrier point by which all promises "should" be handled) or even something as simple as a regular interval (e.g. of a ~10s or something) so that promises have some space to get dealt.
There was a problem hiding this comment.
In this case these promises should be handled when Auspice renders the dataset they represent, which may never happen (e.g. if a narrative never renders the slide backed by this dataset). I think in every other case we attach catch clauses (or use try/catch + async) when the promise is set up, so the default behavior seems fine. At least, I can't remember seeing an "unhandled promise rejection" log which was not indicative of a bug (in the code modified here we were implicitly resolving to undefined, which had its own problems).
jameshadfield
force-pushed
the
inline-root-sequence
branch
from
7204eed
to
500ebc8
Compare
nextstrain-bot
temporarily deployed
to
auspice-inline-root-seq-d0lglm
The previous code would (depending on the sidecar file in question) catch fetch errors and print console errors, and then also attempt to load the (non-existant) data, resulting in more errors. I believe this was due to promise chains such as: promise.then() .catch() .then() .catch() and the (mis)understanding that if the first catch clause was hit then the entire chain would terminate. The approach implemented here also lets us differentiate between fetching + parsing errors, which is a subtle but nice improvement to the warning notifications. (Added comments are also relevant for understanding intended behaviour.)
For smaller genomes it's a nicer experience to inline the root sequence data to avoid having to manage an extra sidecar file, with the downside being an (often negligible) increase in file size. This commit implements this by continuing our approach of conditionally fetching sidecars based on the data present in the main JSON. For full context, see discussions in Slack including <https://bedfordlab.slack.com/archives/C01LCTT7JNN/p1690930305357089> Related Augur PR <nextstrain/augur#1295>
jameshadfield
force-pushed
the
inline-root-sequence
branch
from
500ebc8
to
471f457
Compare
nextstrain-bot
temporarily deployed
to
auspice-inline-root-seq-d0lglm
For smaller genomes it's a nicer experience to inline the root sequence data to avoid having to manage an extra sidecar file, with the downside being an (often negligible) increase in file size. Data structure (schema) is identical between a root-sequence JSON and the top-level JSON key `root_sequence` No sanity checking is done on the genome size, but the intention is for only small genomes to be inlined. For full context, see discussions in Slack including <https://bedfordlab.slack.com/archives/C01LCTT7JNN/p1690930305357089> Related Auspice PR <nextstrain/auspice#1688>
For smaller genomes it's a nicer experience to inline the root sequence data to avoid having to manage an extra sidecar file, with the downside being an (often negligible) increase in file size. Data structure (schema) is identical between a root-sequence JSON and the top-level JSON key `root_sequence` No sanity checking is done on the genome size, but the intention is for only small genomes to be inlined. For full context, see discussions in Slack including <https://bedfordlab.slack.com/archives/C01LCTT7JNN/p1690930305357089> Related Auspice PR <nextstrain/auspice#1688>
See commits for further details (Let me know if anyone would like datasets for testing).
Corresponding Augur PR
Closes #1682