gbv / cocoda

A web-based tool for creating mappings between knowledge organization systems.
https://coli-conc.gbv.de/cocoda/
MIT License
39 stars 5 forks source link

Plugin to manage and integrate data loaded from an API #608

Closed stefandesu closed 2 years ago

stefandesu commented 3 years ago

In Cocoda, we have the so called "CDK mixin". This is a wrapper around cocoda-sdk which deals with pretty much all loading of data inside of Cocoda. There are many reasons for having something like this:

As you can see, there are lots of reasons for having such a plugin, but it's also fairly complex. There are also some things that the "CDK mixin" does not do well. For example, every single thing that was once loaded from the API gets kept in memory forever. There is (currently) no way of detecting which items are actively in use and which items haven't been accessed in some time, so we can't easily clear data from memory. I haven't yet found a solution for this. The consequence is that if you use Cocoda for a long period of time, it will use more and more memory. Fortunately, the data is fairly small, so this hasn't been an issue yet, but eventually this should be solved somehow.

I'd be very happy for any thoughts on this issue. Should this be part of jskos-vue, or maybe a separate package? Have I forgotten anything that we also need in terms of functionality? Any other thoughts or ideas?

nichtich commented 3 years ago

By now the only component that queries a backend API is ItemSuggest. I'd prefer to get all API interaction out of jskos-vue and use plain promises instead, so data loading is decoupled from jskos-vue (see gbv/jskos-vue#14).

stefandesu commented 3 years ago

I mean I did suggest to put it in a separate package, so all my points still stand, and I agree that it make sense to keep API interaction out of jskos-vue and focus it on interface components.

I'm not sure what you mean by "use plain promises instead" though.

nichtich commented 3 years ago

I'm not sure what you mean by "use plain promises instead" though.

If a component of jskos-vue should dynamically load content, then this should be done via a property that is given an async function such as search in ItemSuggest. How this functions gets data is out of the scope of jskos-vue.

stefandesu commented 3 years ago

If a component of jskos-vue should dynamically load content, then this should be done via a property that is given an async function such as search in ItemSuggest. How this functions gets data is out of the scope of jskos-vue.

Yeah that's totally fine with me and makes sense. But it doesn't have anything to do with this issue. 😅

stefandesu commented 3 years ago

I've had some thoughts on this. Currently, all data that is loaded gets "connected" with each other, so if we have a concept A and a child concept A.1, A's narrower property will contain a reference to A.1. This has worked well so far, however, I suspect that because we have so many objects loaded at the same time and all of them are interlinked, and also all of them are reactive when used in Vue, the data gets quite complex. Sometimes, certain properties aren't linked together properly which then leads to old data in the interface. I also think that stuff is updating and rerendering way more often than actually necessary (i.e. when the object actually changes).

I've been experimenting a little bit and may have found a better way to do things. We still have one big items object that has URIs as keys and reactive items as values. All the data is in there. However, instead of interlinking all those objects like before, we only keep the uri property when passing things around, and only where actual access to the object is needed, we can refer to the items object and get the actual data.

This would require changes to how our Vue components work. For example, in ItemName the only property required on the item that is passed in is uri, and then we have a computed value that gives us the actual item from the big items object. I've tested this and it works very well. Only affected items are updated when data is changed, and components only updated if data actually changed. This seems very efficient to me.

I'm not sure how this would work together with our plans to put the Vue components in a separate package (https://github.com/gbv/jskos-vue) because all the components would need to be updated to support this. It should be possible to be implemented without much overhead though. (Maybe we could first implement it only in Cocoda and then transfer those changes to jskos-vue before later replacing Cocoda's components with those from jskos-vue.)

Of course, this requires deep changes to how Cocoda works, but I believe we can achieve much better performance, together with #581.

stefandesu commented 3 years ago

Some more technical considerations:

stefandesu commented 2 years ago

One more reason to actually put this into practice, even though it requires some changes to the application: Due to the nature of the deeply nested objects we are currently using, watchers and computed properties refresh way more often than actually necessary. This is one of the root issues of #633 (and might help a lot with #635 and #636). I think it would have more benefits than expected to go through this change, so I'll give it another try tomorrow!

stefandesu commented 2 years ago

I made good progress today. It's currently possible to test it out on the Test instance (https://coli-conc.gbv.de/cocoda/test/). I wouldn't expect it to work 100% though; there might still be small issues left from the transition. It worked really well on my local setup with a minimalistic config though!

Known issues that need to be fixed:

stefandesu commented 2 years ago

I want to replace the following items in the Vuex store with the URI-only versions:

It might also make sense to include mappings in the items store, but that might get a separate issue.

For some reason, there are also WAY too many mapping requests, and since it's not happening on Dev, it has to be related to this issue...

Edit: favoriteConcepts was reevaluated constantly and subsequently caused loading mappings for those concepts.

stefandesu commented 2 years ago

One big thing is still missing:

stefandesu commented 2 years ago

I have now fixed a bunch of stuff related to this (and probably other stuff unrelated to this as well) and it seems to be working well. I'm still awaiting some testing in the Test instance of Cocoda before closing this issue: https://coli-conc.gbv.de/cocoda/test/

stefandesu commented 2 years ago
stefandesu commented 2 years ago
stefandesu commented 2 years ago

I find selecting a concept to still be fairly slow and I'm having a hard time figuring it out where the bottleneck is. It's actually slower now than it has been in the last release version without all these changes. 😕 So I'm not yet ready to merge this anytime soon, unfortunately.

Edit: It is related to long concept lists (which I've been using for testing) and it seems like the same performance issue when selecting a concept occurs in the last release when using long concept lists. So not an issue related to these changes.

I have found another performance bottleneck which I'm currently working on improving though.

stefandesu commented 2 years ago

Now merged into Dev!