-
Notifications
You must be signed in to change notification settings - Fork 492
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
Update to 6.1 possibly caused loss of dataset thumbnails #10220
Comments
I would close it now, it might be not all icons were recreated and the recreation just needs time... |
Hmm, this is probably due to this PR: Should we add something to the release notes? |
Hi Phil, Case 1: Case 2: Case 3: As I can see, at the moment there are almost no servers running 6.1, except https://data.qdr.syr.edu/ But there are no manually added icons, I suppose. Interestingly, if I manually add thumbnail to the demo system: https://demo.dataverse.org/dataset.xhtml?persistentId=doi:10.70122/FK2/38WUNS It does not appear at all , only if I edit the dataset (you can try, I guess, you have admin account there). https://demo.dataverse.org/dataset-widgets.xhtml?datasetId=2113302 And it looks ok, if automatically generated: |
The bug where a manually-added dataset thumbnail is showing in full-res in the search results, effectively breaking the page - is that reproducible on demo.dataverse.org? Or is that something that only happens with existing thumbnails, when upgrading to 6.1? |
Thats true, I could not reproduce it at dataverse.org. At our system it applies to newly added thumbnails. |
I'm a bit confused by this part of your report - from looking both at the code, and at our own 6.0 instance here at IQSS, it really looks like the dataset-level thumbnails, the ones at the top of the dataset page, have always been 48 px. Was your 6.0 instance a modified, forked version by any chance? |
I am very concerned about the other bug, the huge "thumbnail", if you can call it that, that messes up the search results page. Trying to reproduce it locally... |
Another possibility of course would be that the thumbnails you had cached on disk or s3 just happened to be higher-res (generated outside of Dataverse, for example - ?), and Dataverse was just using them happily instead of the standard 48x ones. But then 6.1 had them re-scaled from scratch - which would be a possibility, since that part of the application was modified in 6.1. |
Uhm, that doesn't appear to be true, actually. It looks like we did make it larger, and variable/configurable at some point. @pdurbin we may have missed this when we were talking about it yesterday (?). This is all turning out to be way more confusing than it should be... |
I can reproduce the "giant thumbnail" issue locally. |
I can reproduce the issue with either filesystem or S3 storage. (And that of course means that it would affect us here at IQSS, if we deployed 6.1 this week, as we were planning to do). |
OK, it appears that custom dataset logos do not work unless there are other images in the dataset. Obviously a bug in itself; even if not the most serious of the bugs we are looking at... But yes, I uploaded another file into the dataset above, published it - and that reproduced the bug just as described. I'm attaching the screenshot from demo, but I will disable that custom thumbnail in the dataset, since we don't want the main page to stay messed up there. Thank you for the report - this appears to be fairly serious. |
Sorry to answer bit late. Seems you found the answers to most questions already. Regarding:
No, we always take the official releases for our productive system and I never changed anything regarding the the thumbnail settings... Here is one example on our productive system: https://bonndata.uni-bonn.de/dataset.xhtml?persistentId=doi:10.60507/FK2/KZHXDF I also looked randomly over other installations, they all seem to have large thumbnails: |
2024/01/17: Added to This Sprint, applied size: 33 during the sprint kickoff |
Producing a proper patch for these bugs will require some work/involve other members of our dev. team. This of course will require a Payara restart. And you most likely will need to shift-reload the page, because browsers usually cache css. |
One extra bit of intel, per @qqmyers is that it may have been the optimization PR #9669 that caused the issue with a larger image, specifically the part mentioned in the description there as
With the commits in question likely being The PR in question added much-needed optimizations to the thumbnail management. But a couple of things like the above just need to be adjusted - should be very doable. |
W.r.t. the small logo size - my guess would be that https://github.com/IQSS/dataverse/blob/f74e4c1c29a6b7381b0a9c416e7228130ed22307/src/main/java/edu/harvard/iq/dataverse/dataset/DatasetUtil.java#L467C57-L467C57 should use DEFAULT_DATASETLOGO_SIZE rather than card size. This method was added late in QA of #9669 and we must have missed testing the published case. (I haven't fully tested but at QDR we use DEFAULT_PREVIEW_SIZE and limit the display to 140px with css because I think we use the same method to return header metadata (unmerged PR #5621) where we want the larger size.) |
@donsizemore from Odum has built a patched .war file with the css workaround included (thank you, Don!): https://github.com/donsizemore/dataverse_backports/releases/download/6.1_css-patch/dataverse-6.1.war |
The three bugs of the apocalypse. The “giant thumbnail bug”, the “tiny thumbnail bug” and the “no thumbnail bug". |
FWIW: W.r.t. the giant thumbs - the code that was replaced in #9669 was dynamically down-scaling every time the page was viewed, and was embedding the image in the page as a base64-encoded string rather than sending a URL that could be cached by the browser. If the css fix above (and the fact that there is a file size limit to avoid truly huge logos being uploaded) isn't enough, I'd suggest perhaps that we do the down-scaling as part of upload and still reference the image via URL so it can be cached. (Hopefully referencing by URL is also better for the SPA than us creating a api call to get the base64-encoded image string.) |
I just said on slack that I'd be ok with adopting the css fix as the actual solution; because of the size limit.
So, I'm perfectly fine with keeping the URL solution in place. But it just looks like that api reads and returns the contents of the |
The patch works great! Is there a way to change the size of automatically generated thumbs as well? Maybe some settings? |
Somebody (namely, @jp-tosca) is working on a fix for that bug as well. There are no similarly quick fixes, from what I understand though - i.e. it should still be simple enough, but will require a code change. |
I may have found the 4th horseman... @landreev It seems that when a file is uploaded thumbnail of the customized thumbnail is overwritten, I am still figuring out the behavior sadly having the dataset_logo.thumb48 won't let us have a fix for this until I figure out what is exactly happening to these images. |
The difference from the "small/big" thumbnail is caused because when the DS is on draft still uses the base64 conversion but when it is on non-draft it uses the API version, this image block has a max-width and max-height of 140 px and because of this the image doesn't break the entire interface as it does on the search page. |
Following with the solution for the main issue it may involve a change where it calls the /thumbnail, not the /logo endpoint but still have to dig a little bit more into it but we may be close to a solution at least for the first and most critical issue. |
I'm not sure I understand the above, tbh. The part about the 4th bug; or the last comment, about a different endpoint. What about Jim's comment above, from 5 days ago ("W.r.t. the small logo size - my guess would be that ...")? - it appears to imply that it may just be a wrong size in one specific code line that needs to be changed - no? |
It seems there is another endpoint to bring the thumbnail so I am not sure we need to change right now the behavior of the endpoint but to switch the JSF to use the other one, if you change the source of the image from api/datasets/{id}/logo to api/datasets/{id}/thumbnail it will show the size that we need for the search, look at the image for an example: The 4th bug if it exists I will documented at a later point, just wanted to bring awareness that could be more involved. |
I see, so this is still about the "giant thumbnail" bug, got it. That's good to know. I was mainly asking about the "tiny thumbnail [as derived from an image Datafile]" bug on the dataset page. It just sounded like there was at least a possibility of a very simple fix for that one, if I was reading Jim's comment correctly. |
Yeah, if there's another bug, I'd like to see a reproducer/curious to know what it is. I'm not surprised that there could be more bugs. I've been saying this forever, that this whole system of thumbnails is way too complicated; probably unnecessarily so, compared to the actual value of thumbnails as a feature. |
If simply switching to And, as I mentioned earlier, I'm less worried about this particular bug (the "giant thumbnail" one), since we already have a css workaround for it. |
I tested this change on DatasetUtil with both values and this didn't help. It seems the code from 441 is not being executed, I will get back when I find more. |
I am adding a PR in a few adding the CSS that @landreev proposed + some padding to the right, otherwise horizontal oriented thumbnails will look something like this: I will also apply the fix that @qqmyers recommended, that will specifically address the issue that @landreev discovered with the small-medium thumbnails. After that I will create an issue for the behavior when sometimes you don't get a thumbnail, I will check on 6.0 if this used to happen and add that to the post. If you guys think we are missing something please let me know. |
Hmm. Wondering why |
@sergejzr We now have a patched .war file with the fixes for this, and a couple of other issues (#10251 and #10200) available at https://github.com/donsizemore/dataverse_backports/releases/tag/6.1_20240126 (thank you, @donsizemore!). These bug fixes have already gone through our normal testing/QA process. We are still offering this build as a prototype at this point, so, if you choose to deploy it, please test it carefully and let us know asap if you encounter any problems. |
Patched .war file fixes this for us, see https://heidata.uni-heidelberg.de/dataverse/arthistoricum |
I went to look, and had a scare moment - because all the thumbnails on the page still looked huge!! - That of course was because I had been to your site in the last few days, and my browser was caching the old css. Shift-reload fixed it, so, all is good. |
Looks very good, thank you!! Also the "old" thumbnails are back. I will let it on our test-server for some time and move to production later on. |
Dear Dataverse Developers,
It seems that after updating 6.0 to 6.1there are several issues with thumbnails on our system. For example the manually created dataset thumbnails are gone and automatically created ones are low resolution. If I manually add large thumbnails, they are also large in the result list, destroying the web page. Here detailed cases with links to our test-system:
Case 1:
Disappeared icon: https://bonndata-test.hrz.uni-bonn.de/dataset.xhtml?persistentId=doi:10.80624/FK2/9S7RJF
Icon added in 6.0, disappeared after update to 6.1. The icon however it still there if I open edit the thumbnail of the dataset. It does not appear even after editing the metadata and re-publishing
Case 2:
Auto-generated: https://bonndata-test.hrz.uni-bonn.de/dataset.xhtml?persistentId=doi:10.80624/FK2/WOX4YV
The thumbnail was automatically generated from pdf of the dataset files in 6.0. After update to 6.1 it became tiny resolution. Generally all automatically generated icons have tiny resolution now....
Case 3:
Dataset with manually added large thumbnail in 6.1. https://bonndata-test.hrz.uni-bonn.de/dataset.xhtml?persistentId=doi:10.80624/FK2/0OBMAR
The thumbnail appear ok in the dataset page, but in search results it is remaining huge. https://bonndata-test.hrz.uni-bonn.de/dataverse/sandbox
Since I am not sure whether there could be other source of error (I could not find any yet) I would like just to ask those who did update to check if they encounter the same..
PS: At the moment, I rolled back our productive installation back to 6.0 and try to find the cause on the test system.
Best regards
Sergej
The text was updated successfully, but these errors were encountered: