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

Add BioJS plugin for multiple sequence alignments #2861

Closed
wants to merge 27 commits into from

Conversation

guerler
Copy link
Contributor

@guerler guerler commented Aug 25, 2016

biojs

Can be tested with a multiple sequence alignment file in text format and by selecting Charts from its visualization options e.g. http://rostlab.org/~goldberg/jalv_example.clustal. More options will be provided in the customization form soon within this PR.
ping @bgruening

@jgoecks
Copy link
Contributor

jgoecks commented Aug 25, 2016

I'm not convinced that we want to add 3rd party visualizations to our framework code repository. I'd prefer to see visualizations—including ours—live in their own repo with scripts and/or admin UI functionality to add them to an instance.

What do others think?

@dannon
Copy link
Member

dannon commented Aug 25, 2016

Considering the effort we took to migrate tools out of the core repository, I think I'm agreeing with @jgoecks on this one -- ideally pluggable stuff like this lives in separate repos.

@hexylena
Copy link
Member

Ideally, yes.

Ideally, the TS (or some alternative) supporting viz (#1191) and datatypes would be on the roadmap and have resources allocated to them.

I'm pretty sure no one wants any of the above in core if we had a better place to put it, and we'd all be very happy to see it moved out. Perhaps we could move this discussion to the roadmap or a similar venue (e.g. #1191) which might be more appropriate to finding resources for solving the root cause?

@guerler
Copy link
Contributor Author

guerler commented Aug 25, 2016

I totally agree @jgoecks, particularly now that we have generalized charts and are technically able to add many new 3rd party visualizations i.e. charts.js which you suggested during the GCC is on the list too, but adding them like this is tricky. The underlying problem however is that we do no have a framework to make these individually installable otherwise we might be able to add them to the toolshed.

@dannon
Copy link
Member

dannon commented Aug 25, 2016

What's the scope of the audience for this particular viz? I guess that's what I'd look at to decide. Given that we're decided(-ish?) on viz's being in the toolshed, if we put it in core now, we'll likely just have to move it out again later, which is a pain.

If we think most or all galaxy instances will want this functionality 'out of the box', I guess I could see an argument to include it similar to how we've left the text manipulation tools in the base distribution.

That said, it's possible to install these things pretty easily manually since they're all javascript and have no dependencies, right? So the real thing we're addressing here is discoverability?

@hexylena
Copy link
Member

If we think most or all galaxy instances will want this functionality 'out of the box', I guess I could see an argument to include it similar to how we've left the text manipulation tools in the base distribution.

If we want that sort of functionality, wouldn't it be better to ship a default list of {tools, viz, datatypes} to bootstrap galaxies with on first run? Given that we do essentially that with the docker images and some ansible code, this should be a smaller problem going forward. Then we could give users and admins complete control over it rather than making the decision on what they want out of the box, for them.

@dannon
Copy link
Member

dannon commented Aug 25, 2016

@erasche Admins have complete control, always, but what comes 'out of the box' is a balancing act, right? One of the awesome things about Galaxy when I very first tried it ~7 years ago was how I literally just did 'hg clone', 'run.sh', and I was able to do things right out of the box.

@bgruening
Copy link
Member

@guerler first of all thanks for working on this. It is awesome!
Maybe you can make it a little bit more clear what this actually mean. As far as I understood the code you are working towards a very easy mechanism to get BioJS plugins into Galaxy, correct?

@dannon you question is getting more and more difficult to answer. Galaxy is approaching complete new areas and we don't have a definition or measurement to answer what "most or all galaxy instances want".

Imho the real problem here is that we all see the need to make Galaxy more like a framework but we don't have the mechanism to do this now. For example I got a few request to also remove the few text manipulation tools from the Docker container, or to remove the dbkey from everywhere.... The first use-case is now supported the last one is way more tricky.

Regarding Gregs visualization, it turns our this is useful for imaging - which again is bioinformatics and coming more and more to Galaxy. So we never now if this is useful and to which degree :)

@dannon
Copy link
Member

dannon commented Aug 25, 2016

@bgruening I totally agree it's great work. The question about what 'most or all galaxy instances want' bit was a poor attempt to ascertain the value of including this in the core codebase now, as compared to the effort required of an administrator doing 'git clone guerler/cool_viz; cp cool_viz galaxy/config/plugins/viz/' which seems to have derailed the conversation a bit.

@bgruening
Copy link
Member

@dannon: to your specific question, I see the MSA viewer as nice addition and can see a lot of use-cases. All motif search people will love it and we have also some tools that supports it. ChIP, CLIP, SELEX ...

doing 'git clone <guerler/cool_viz; cp cool_viz galaxy/config/plugins/viz/' which seems to have derailed the conversation a bit.

You mean RUN install-biojs msa from https://github.com/bgruening/docker-galaxy-stable#extending-the-docker-image :) It's easy enough for admins - and it does not need to be in core. The only reason is visibility and the community aspect.
Together with the vis-repo mentioned by @guerler this seems to be a good way.

@bgruening
Copy link
Member

xref: For a few more visualization that we keept out of core: https://github.com/bgruening/galaxytools/tree/master/visualisations

@jgoecks
Copy link
Contributor

jgoecks commented Aug 26, 2016

I don't think this visualization is broadly useful enough to go into the framework code. However, I said that about the chat server code as well, and it's now in the framework, so perhaps I don't have a good sense of what's broadly useful.

Something to consider: if we agree that most/all visualizations should be pluggable long-term and exist in their own repo(s), then we are creating technical debt by introducing them into the core repo now, because they will have to be removed in the future. So the question becomes whether the time "repaying" the technical debt we're introducing now would be better spent taking steps toward doing the right thing, like working towards visualizations in the toolshed or creating a script so we can bootstrap visualizations from initial instance startup.

@bgruening
Copy link
Member

I don't think this visualization is broadly useful enough

I don't think we can or should judge this. If we start with this we will scare entire communities away. If we fear the migration effort later, we should either say 1) no visualization at all - similar to tools or 2) let get them in - similar to datatypes.

  1. means we need to have an infrastructure to support it. What do we in the interim?

Why does 2) means we need to migrate them at some point? Is this our only concern? Can we make it critical clear that visualizations can disappear at some point? Can we include an option to galaxy.ini that we will get all community visualizations at once?

Btw. this also holds true for IEs as well. Is Jupyter really interesting for our target user? Obviously I would say yes, but many others probably see this differently.

@jgoecks
Copy link
Contributor

jgoecks commented Aug 27, 2016

Based on discussion in #1191, there is consensus that 1)—no visualizations in the core—is the preferred approach. This is reasonable to me when comparing to datatypes because visualizations are much more substantial, with potential dependencies and installation required.

As a stop gap until we can accomplish a real visualization registry, I would prefer (in order):

  1. a script that installs some/all community visualizations;
  2. an option in galaxy.ini that installs some/all community visualizations;
  3. an option/menu in the admin UI that installs some/all community visualizations. (This is my 3rd choice because implementing this would be best with the others requirements @jmchilton lists in the Allow installation of visualizations from toolshed #1191 discussion.)

@guerler
Copy link
Contributor Author

guerler commented Aug 31, 2016

Closing in favor of #2875

@guerler guerler closed this Aug 31, 2016
@jmchilton
Copy link
Member

I'm fine with closing this PR and taking a different route - but I find the word "consensus" to be a bit strong so I did +1 this before it was closed. My preferred path would be to merge these into core until we have something better to do with them and I don't think I'm alone.

@guerler
Copy link
Contributor Author

guerler commented Aug 31, 2016

I am fine with either solution. The PR I replaced it with currently accesses the chart types from an external repo but can be redirected to a local directory too (like before). Each chart type consists of three files, a js-bundle, a js-config and a png-logo. I moved these files to the repo somewhat preemptively, so we can do whatever the consensus is/will be. It also demonstrates that either works fine. I think there is some confusion between these limited and strictly js-based chart types and the more generic visualization plugins framework of Galaxy. The entire charts plugin is a single visualization within the framework of Galaxy, while the chart types we are discussing here are subcomponents of charts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants