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

Treenome Browser v1 integration #302

Merged
merged 78 commits into from
Aug 12, 2022
Merged

Conversation

amkram
Copy link
Collaborator

@amkram amkram commented Jul 20, 2022

Here's a PR where we can iron out the final details of the treenome browser.

A couple recent additions:

  • added nt mutations (now the mutations displayed in the browser match those selected in the cog menu). Mutations also load progressively in "chunks" rather than all at once.
  • added a band that appears at the selected node when zoomed in far enough, highlighting that node's genome

I think the last main thing to figure out is, as you mentioned, handling non-SARS-CoV-2 trees.

Right now it really only works for the cov2tree tree. The two main things needed to build the browser for an arbitrary tree are (1) the reference sequence and (2) coordinates of genes/CDS to compute nt positions of aa mutations. (2) is only necessary for aa mutations, so just displaying nt mutations could work without it.

I currently compute the reference through the fake "mutations" at the root node -- which I realize aren't guaranteed to be there (but I think they should be if a .gb file was used to make the tree)

(2) is hard-coded for SARS-CoV-2. But if we stored the CDS data from a .gb file, I could use those.

What do you think about storing the genbank CDS features (potentially also the fasta sequences) in the jsonl file, and only displaying the treenome toggle button if they are present?

@amkram
Copy link
Collaborator Author

amkram commented Aug 8, 2022

Ok, latest commit should fix that horizontal zoom issue!

Copy link
Owner

@theosanderson theosanderson left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for all the hard work!

@theosanderson
Copy link
Owner

Oh yeah I remember this thing about the build failure to Docker. I looked into that a bit before: it's because of a canvas dependency from JBrowse. Will have a think about that..

@theosanderson
Copy link
Owner

theosanderson commented Aug 10, 2022

Oh yeah I remember this thing about the build failure to Docker. I looked into that a bit before: it's because of a canvas dependency from JBrowse. Will have a think about that..

What might work is to: set https://github.com/theosanderson/taxonium/blob/master/Dockerfile.frontend#L1 to node:18.0.0-alpine which should be v102. That should then let us use canvas-v2.9.3-node-v102-linux-glibc-x64.tar.gz if we pin canvas to 2.9.3

@amkram
Copy link
Collaborator Author

amkram commented Aug 11, 2022

It seems like it is due to the fact that alpine uses musl instead of glibc, and there is no musl build for canvas. I think using e.g. a bullseye image instead of alpine would probably solve that, if you are open to the switch? Otherwise I think we can maybe build canvas from source

@theosanderson
Copy link
Owner

Ah good catch – definitely fine with that!

@theosanderson
Copy link
Owner

I'm not depending on the Docker image at the moment -- that was in use on a version within GISAID but that project seems to have stalled -- so feel free to merge this without fixing that and I can try to figure it out in due course

@amkram
Copy link
Collaborator Author

amkram commented Aug 11, 2022

Looks like the image is building okay now.

Another issue was that canvas doesn't have builds for Node 18 yet. As far as I can tell, in the releases, node-v102 corresponds to Node 16, and node-v108 would be Node 18 (if it were present). I downgraded the docker image to Node 16 which builds successfully.

Let me know if you think downgrading to 16 is an issue.

I don't know enough about node/docker to know why this was only an issue in the Docker build...

@theosanderson
Copy link
Owner

theosanderson commented Aug 11, 2022

node-v102 corresponds to Node 16, and node-v108 would be Node 18 (if it were present). I downgraded the docker image to Node 16 which builds successfully.

Apologies that I got this wrong!

Let me know if you think downgrading to 16 is an issue.

Hmm interesting. It definitely would be an issue for the backend, which uses the fetch API (which is normally in the browser, not node, but was added experimentally in node 17 and properly in node 18). Somehow I convinced myself it was needed generally but I think you're right that it's not.

I don't know enough about node/docker to know why this was only an issue in the Docker build...

Yeah me too :(

@amkram
Copy link
Collaborator Author

amkram commented Aug 11, 2022

Ok, I think this should do it! Went back to Node 18 alpine, and now canvas is built from source first before installing everything else. It does unfortunately take quite a bit longer to build now (~5m).

BTW, should the integration tests be failing? I think it's due to a permissions issue (I had removed my write access so I wouldn't accidentally push to main again 😬 )

@theosanderson
Copy link
Owner

BTW, should the integration tests be failing? I think it's due to a permissions issue (I had removed my write access so I wouldn't accidentally push to main again 😬 )

Yeah this is permissions but not to worry -- I think it will be fine, we can definitely merge as is. Are you happy for me to merge now?

@amkram
Copy link
Collaborator Author

amkram commented Aug 12, 2022

Awesome. I am ready if you are!

@theosanderson
Copy link
Owner

OK, let's see how this goes! Thanks for the amazing feature and working through all these points.

@theosanderson theosanderson merged commit 7176011 into theosanderson:master Aug 12, 2022
@theosanderson
Copy link
Owner

🎉 🎉 everything seems to be working, tests passing, etc.

@amkram
Copy link
Collaborator Author

amkram commented Aug 12, 2022

Yay! Thanks for all of your help and patience!

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.

2 participants