Fix for 'Error reading from name store' #820

Merged
merged 3 commits into from Jan 30, 2017

Conversation

Projects
None yet
6 participants
@abretaud
Contributor

abretaud commented Oct 17, 2016

Hi!
This is a fix for the error message 'Error reading from name store' I get when opening a JBrowse instance for the first time, or clicking on "Go" with random stuff in the search box.
I'm not sure if this is very elegant, or if it could have some side effect, but I thought it was worth opening the PR.

@abretaud abretaud referenced this pull request in galaxyproject/tools-iuc Oct 17, 2016

Merged

Jbrowse: new tool + contextual menus #994

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Oct 17, 2016

Contributor

This is sort of "silencing" the error message, which is something that might need fixing at some other part of the name indexing pipeline. It would be good to get a solid test case of something that produces the "Error reading name store" issue and then analyze that

Contributor

cmdcolin commented Oct 17, 2016

This is sort of "silencing" the error message, which is something that might need fixing at some other part of the name indexing pipeline. It would be good to get a solid test case of something that produces the "Error reading name store" issue and then analyze that

@abretaud

This comment has been minimized.

Show comment
Hide comment
@abretaud

abretaud Oct 17, 2016

Contributor

Yes, there's probably a more clever way.

I can reproduce the error with launching a docker-jbrowse with jbrowse 1.12.1, load a test genome with only one scaffold, generate-names.pl, then go to jbrowse => I get the error

From what I understood, jbrowse see that there is "scaffold1:1..23454" in the search box, make an hash of it, then an ajax request to the corresponding name file, and it gets a 404 error (as scaffold1:1.23454 is not in the index) which triggers the error message.

With my patch, in case of error (404 or other), it returns empty result and jbrowse continue to the other method = parsing the search box content as a location (which works).

Contributor

abretaud commented Oct 17, 2016

Yes, there's probably a more clever way.

I can reproduce the error with launching a docker-jbrowse with jbrowse 1.12.1, load a test genome with only one scaffold, generate-names.pl, then go to jbrowse => I get the error

From what I understood, jbrowse see that there is "scaffold1:1..23454" in the search box, make an hash of it, then an ajax request to the corresponding name file, and it gets a 404 error (as scaffold1:1.23454 is not in the index) which triggers the error message.

With my patch, in case of error (404 or other), it returns empty result and jbrowse continue to the other method = parsing the search box content as a location (which works).

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Oct 18, 2016

Contributor

On further investigation, I think this is actually a good solution. I looked into trying to fix the fact that it queries the name store for the "location string"(e.g. scaffold1234:1..500) but basically there are codepaths for first querying the name store and then falling back to treating it as a location, which seems fine. Using this solution that just handles the 404 case seems good(you could check specifically for a 404 error code but that doesn't seem super essential)

Contributor

cmdcolin commented Oct 18, 2016

On further investigation, I think this is actually a good solution. I looked into trying to fix the fact that it queries the name store for the "location string"(e.g. scaffold1234:1..500) but basically there are codepaths for first querying the name store and then falling back to treating it as a location, which seems fine. Using this solution that just handles the 404 case seems good(you could check specifically for a 404 error code but that doesn't seem super essential)

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Oct 18, 2016

Contributor

I also noticed what could be a bug while looking through the existing code but I'm not sure how often it causes issues. I think the promise's "then" error condition is not correct and could be fixed by this patch

diff --git a/src/JBrowse/Browser.js b/src/JBrowse/Browser.js
index 6e1298c..09e8465 100644
--- a/src/JBrowse/Browser.js
+++ b/src/JBrowse/Browser.js
@@ -2303,7 +2303,9 @@ navigateTo: function(loc) {
                     className: 'notfound-dialog'
                 }).show();
             },
-            thisB.callLocation(loc));
+            function() {
+                thisB.callLocation(loc)
+            });
     });
 },

If you wanted to add that to the PR that would be great

Contributor

cmdcolin commented Oct 18, 2016

I also noticed what could be a bug while looking through the existing code but I'm not sure how often it causes issues. I think the promise's "then" error condition is not correct and could be fixed by this patch

diff --git a/src/JBrowse/Browser.js b/src/JBrowse/Browser.js
index 6e1298c..09e8465 100644
--- a/src/JBrowse/Browser.js
+++ b/src/JBrowse/Browser.js
@@ -2303,7 +2303,9 @@ navigateTo: function(loc) {
                     className: 'notfound-dialog'
                 }).show();
             },
-            thisB.callLocation(loc));
+            function() {
+                thisB.callLocation(loc)
+            });
     });
 },

If you wanted to add that to the PR that would be great

@abretaud

This comment has been minimized.

Show comment
Hide comment
@abretaud

abretaud Oct 18, 2016

Contributor

Nice, I just pushed your fix!
And I also changed the code to only silently ignore 404 errors (I guess having an error in case of 403 could be useful for admins)

Contributor

abretaud commented Oct 18, 2016

Nice, I just pushed your fix!
And I also changed the code to only silently ignore 404 errors (I guess having an error in case of 403 could be useful for admins)

@erasche

This comment has been minimized.

Show comment
Hide comment
@erasche

erasche Nov 14, 2016

Contributor

Would be very interested in having this feature in the next release! This error plagued us enough we turned off the (useful) feature.

Contributor

erasche commented Nov 14, 2016

Would be very interested in having this feature in the next release! This error plagued us enough we turned off the (useful) feature.

@abretaud

This comment has been minimized.

Show comment
Hide comment
@abretaud

abretaud Dec 15, 2016

Contributor

Hi,
Any chance to get this fix included in the next JBrowse?

Contributor

abretaud commented Dec 15, 2016

Hi,
Any chance to get this fix included in the next JBrowse?

@abretaud

This comment has been minimized.

Show comment
Hide comment
@abretaud

abretaud Jan 30, 2017

Contributor

A little ping in case this PR got out of your radar, it would very cool to have this fix in the next release!

Contributor

abretaud commented Jan 30, 2017

A little ping in case this PR got out of your radar, it would very cool to have this fix in the next release!

@nathandunn

This comment has been minimized.

Show comment
Hide comment
@nathandunn

nathandunn Jan 30, 2017

Contributor

@abretaud I'm sure it will be in the next JBrowse release. Do you want it in the next Apollo release? It makes sense to me.

If at least two of you have tested it, then I'll go ahead and merge it in and bump it for the Apollo release as well.

Contributor

nathandunn commented Jan 30, 2017

@abretaud I'm sure it will be in the next JBrowse release. Do you want it in the next Apollo release? It makes sense to me.

If at least two of you have tested it, then I'll go ahead and merge it in and bump it for the Apollo release as well.

@abretaud

This comment has been minimized.

Show comment
Hide comment
@abretaud

abretaud Jan 30, 2017

Contributor

Oh yes, if it's possible for next apollo release that would be great!

Contributor

abretaud commented Jan 30, 2017

Oh yes, if it's possible for next apollo release that would be great!

@nathandunn

This comment has been minimized.

Show comment
Hide comment
@nathandunn

nathandunn Jan 30, 2017

Contributor

@cmdcolin @erasche @enuggetry Has someone else tested this fix successfully? We are doing our last round of testing (knock on wood) for Apollo and would be great to get this in, if possible.

Contributor

nathandunn commented Jan 30, 2017

@cmdcolin @erasche @enuggetry Has someone else tested this fix successfully? We are doing our last round of testing (knock on wood) for Apollo and would be great to get this in, if possible.

@erasche

This comment has been minimized.

Show comment
Hide comment
@erasche

erasche Jan 30, 2017

Contributor

@nathandunn yep, tested a while back, was satisfied that it fixed the issue.

Contributor

erasche commented Jan 30, 2017

@nathandunn yep, tested a while back, was satisfied that it fixed the issue.

@nathandunn nathandunn merged commit 2c0c207 into GMOD:master Jan 30, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nathandunn

This comment has been minimized.

Show comment
Hide comment
@nathandunn

nathandunn Jan 30, 2017

Contributor

@abretaud merged and will be in Apollo 2.0.6

Contributor

nathandunn commented Jan 30, 2017

@abretaud merged and will be in Apollo 2.0.6

@abretaud

This comment has been minimized.

Show comment
Hide comment
@abretaud

abretaud Jan 31, 2017

Contributor

Cool! Many thanks!

Contributor

abretaud commented Jan 31, 2017

Cool! Many thanks!

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Feb 8, 2017

Contributor

I think this has an issue now where it will snap to the start of the chromosome regardless of what was in &loc= in the URL, but only if generate-names.pl hasn't been run

The change I suggested, specifically this one, needs reverting

diff --git a/src/JBrowse/Browser.js b/src/JBrowse/Browser.js
index 6e1298c..09e8465 100644
--- a/src/JBrowse/Browser.js
+++ b/src/JBrowse/Browser.js
@@ -2303,7 +2303,9 @@ navigateTo: function(loc) {
                     className: 'notfound-dialog'
                 }).show();
             },
-            thisB.callLocation(loc));
+            function() {
+                thisB.callLocation(loc)
+            });
     });
 },
Contributor

cmdcolin commented Feb 8, 2017

I think this has an issue now where it will snap to the start of the chromosome regardless of what was in &loc= in the URL, but only if generate-names.pl hasn't been run

The change I suggested, specifically this one, needs reverting

diff --git a/src/JBrowse/Browser.js b/src/JBrowse/Browser.js
index 6e1298c..09e8465 100644
--- a/src/JBrowse/Browser.js
+++ b/src/JBrowse/Browser.js
@@ -2303,7 +2303,9 @@ navigateTo: function(loc) {
                     className: 'notfound-dialog'
                 }).show();
             },
-            thisB.callLocation(loc));
+            function() {
+                thisB.callLocation(loc)
+            });
     });
 },
@abretaud

This comment has been minimized.

Show comment
Hide comment
@abretaud

abretaud Feb 8, 2017

Contributor

Oh, I hadn't seen this problem
Do you want me to create another PR?

Contributor

abretaud commented Feb 8, 2017

Oh, I hadn't seen this problem
Do you want me to create another PR?

cmdcolin added a commit that referenced this pull request Feb 8, 2017

@cmdcolin

This comment has been minimized.

Show comment
Hide comment
@cmdcolin

cmdcolin Feb 8, 2017

Contributor

I went ahead and added commit to master to fix. It still has your fix too, so it should be best of both worlds.

I'd consider the bug that was introduced pretty important, enough to warrant update (again, the behavior was that page refresh for example would always take you back to start of chromosome if generate-names.pl wasn't run).

I'd consider updating tags/commits to include the latest fix

Contributor

cmdcolin commented Feb 8, 2017

I went ahead and added commit to master to fix. It still has your fix too, so it should be best of both worlds.

I'd consider the bug that was introduced pretty important, enough to warrant update (again, the behavior was that page refresh for example would always take you back to start of chromosome if generate-names.pl wasn't run).

I'd consider updating tags/commits to include the latest fix

@erasche erasche referenced this pull request in galaxyproject/tools-iuc Apr 26, 2017

Closed

JBrowse: Error reading from name store #1277

@terrymun

This comment has been minimized.

Show comment
Hide comment
@terrymun

terrymun Apr 8, 2018

Just want to say that I have somehow managed to run into this issue, too, but only with a specific reference sequence I am using. Somehow using it with other reference sequences never threw a problem.

I am using a heavily-modded version of v1.12.0, so upgrading it to the latest release was a big too much work at the moment. I have however managed to sneak in the proposed changes directly into dojo.js and that stopped the error message from popping up when JBrowse is being loaded.

Reproducing the issue

How to reproduce the problem:

  • Load a problematic reference sequence (I still don't know what makes a reference sequence "problematic" compared to others)
  • Run bin/prepare-refseqs.pl ...
  • Run bin/bin/generate-names.pl...
  • Load up the reference sequence by providing a specific value to the loc query string that must contain a position, e.g. chr1:1:5000. Using loc=chr1 actually stops the error from showing up.

When I check the console log, I can see that JBrowse is attempting to fetch a non-existent .json file.

Solutions that worked for other cases, but did not work to resolve the issue

Proposed solutions that did not work:

  • Using --hashBits 16 (or any values between 12–16 as suggested)
  • Specifying --tracks DNA in the option

Additional observations: the "saturation" hypothesis

One thing I did notice is the following:

  • Reference sequences that generate a huge list of files in the /names folder will not have an issue. It seems like a "saturation" thing, as the folder will contain folders from 000 to fff, and each containing 0.json, 1.json... through f.json. Alternatively, /names folder that do not have folders but only .json files directly also are error-free.

names-with-files--saturated
names-with-subfolders--saturated

  • Reference sequences that do not have all the above permutations will have an issue with the names store.

names-with-files--unsaturated

Possible cause

When I looked into the individual contents of these files, I realised that the non-problematic reference sequences had a lot of "entities" attached to it, in the form of track items. I believe that these track items saturate the indexer and causes the indexer to "cover all bases", creating all possible .json files with 3-character hex-coded file names. Therefore, JBrowse never runs into an issue where it fails to find a non-existent .json file.

Meanwhile, the problematic reference sequences do not have any tracks (or very little tracks with very little features), which causes the indexer to be "undersaturated", which means 3-character file/folder names are not generated without using up all possible combinations. Eventually, JBrowse will run into loading a file that does not exist.

terrymun commented Apr 8, 2018

Just want to say that I have somehow managed to run into this issue, too, but only with a specific reference sequence I am using. Somehow using it with other reference sequences never threw a problem.

I am using a heavily-modded version of v1.12.0, so upgrading it to the latest release was a big too much work at the moment. I have however managed to sneak in the proposed changes directly into dojo.js and that stopped the error message from popping up when JBrowse is being loaded.

Reproducing the issue

How to reproduce the problem:

  • Load a problematic reference sequence (I still don't know what makes a reference sequence "problematic" compared to others)
  • Run bin/prepare-refseqs.pl ...
  • Run bin/bin/generate-names.pl...
  • Load up the reference sequence by providing a specific value to the loc query string that must contain a position, e.g. chr1:1:5000. Using loc=chr1 actually stops the error from showing up.

When I check the console log, I can see that JBrowse is attempting to fetch a non-existent .json file.

Solutions that worked for other cases, but did not work to resolve the issue

Proposed solutions that did not work:

  • Using --hashBits 16 (or any values between 12–16 as suggested)
  • Specifying --tracks DNA in the option

Additional observations: the "saturation" hypothesis

One thing I did notice is the following:

  • Reference sequences that generate a huge list of files in the /names folder will not have an issue. It seems like a "saturation" thing, as the folder will contain folders from 000 to fff, and each containing 0.json, 1.json... through f.json. Alternatively, /names folder that do not have folders but only .json files directly also are error-free.

names-with-files--saturated
names-with-subfolders--saturated

  • Reference sequences that do not have all the above permutations will have an issue with the names store.

names-with-files--unsaturated

Possible cause

When I looked into the individual contents of these files, I realised that the non-problematic reference sequences had a lot of "entities" attached to it, in the form of track items. I believe that these track items saturate the indexer and causes the indexer to "cover all bases", creating all possible .json files with 3-character hex-coded file names. Therefore, JBrowse never runs into an issue where it fails to find a non-existent .json file.

Meanwhile, the problematic reference sequences do not have any tracks (or very little tracks with very little features), which causes the indexer to be "undersaturated", which means 3-character file/folder names are not generated without using up all possible combinations. Eventually, JBrowse will run into loading a file that does not exist.

@rbuels

This comment has been minimized.

Show comment
Hide comment
@rbuels

rbuels Apr 9, 2018

Collaborator

@terrymun are you able to reproduce the issue with a non-modded JBrowse? like a git checkout, or one of the -dev releases? if so, you could just tar up the whole thing and send it to me so I can track down what's going on

Collaborator

rbuels commented Apr 9, 2018

@terrymun are you able to reproduce the issue with a non-modded JBrowse? like a git checkout, or one of the -dev releases? if so, you could just tar up the whole thing and send it to me so I can track down what's going on

@terrymun

This comment has been minimized.

Show comment
Hide comment
@terrymun

terrymun Apr 11, 2018

@rbuels I will try to do that by the end of the week: I'm curious by what is actually happening! I've modded only the frontend facing part of JBrowse though, didn't touch any of the build scripts :) I will of course use the unmodified version and see if I can reproduce it reliably ;)

Do you recommend that I use v1.12.0 to reproduce the issue, or the latest release?

terrymun commented Apr 11, 2018

@rbuels I will try to do that by the end of the week: I'm curious by what is actually happening! I've modded only the frontend facing part of JBrowse though, didn't touch any of the build scripts :) I will of course use the unmodified version and see if I can reproduce it reliably ;)

Do you recommend that I use v1.12.0 to reproduce the issue, or the latest release?

@terrymun

This comment has been minimized.

Show comment
Hide comment
@terrymun

terrymun Apr 13, 2018

@rbuels I've managed to reproduce the issue with the latest release of JBrowse. Of course, the name store error has been manually suppressed such that a 404 status no longer triggers a modal opening, but I can still see an invalid request being made to load a JSON file that does not exist for an "unsaturated" genome.

I have set up a temporary test case on a server I've been working on. Open the browser's dev tools and you can find the errors logged for the "unsaturated" version:

The "saturated" genome contains an additional track with 500,000+ features, and the high density of features saturates the names store, so all possible JSON files will be produced—and no 404 errors. Meanwhile, the "unsaturated" genome only contains the DNA track.

Can you provide me an email to send the link of the gzipped archive to you? I've sent an email to the one you've listed in your GitHub profile, via a Danish e-infrastructure file sharing service. I will bundle have bundled in a script that setups up the two test cases alongside with raw data for you to test it locally :) more details are in the email.

terrymun commented Apr 13, 2018

@rbuels I've managed to reproduce the issue with the latest release of JBrowse. Of course, the name store error has been manually suppressed such that a 404 status no longer triggers a modal opening, but I can still see an invalid request being made to load a JSON file that does not exist for an "unsaturated" genome.

I have set up a temporary test case on a server I've been working on. Open the browser's dev tools and you can find the errors logged for the "unsaturated" version:

The "saturated" genome contains an additional track with 500,000+ features, and the high density of features saturates the names store, so all possible JSON files will be produced—and no 404 errors. Meanwhile, the "unsaturated" genome only contains the DNA track.

Can you provide me an email to send the link of the gzipped archive to you? I've sent an email to the one you've listed in your GitHub profile, via a Danish e-infrastructure file sharing service. I will bundle have bundled in a script that setups up the two test cases alongside with raw data for you to test it locally :) more details are in the email.

@rbuels

This comment has been minimized.

Show comment
Hide comment
@rbuels

rbuels Apr 16, 2018

Collaborator

@terrymun I see that indeed it is getting 404s when trying to fetch some of the names files, which is actually normal for "unsaturated" names stores. Does the name searching still work, from the user's perspective? This is a big thread, I'm a little unclear whether the main concern is the 404s (normal) or the name searching failing to work at all (not normal).

Collaborator

rbuels commented Apr 16, 2018

@terrymun I see that indeed it is getting 404s when trying to fetch some of the names files, which is actually normal for "unsaturated" names stores. Does the name searching still work, from the user's perspective? This is a big thread, I'm a little unclear whether the main concern is the 404s (normal) or the name searching failing to work at all (not normal).

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