GateNLP / gate-core

The GATE Embedded core API and GATE Developer application
GNU Lesser General Public License v3.0
75 stars 29 forks source link

xgapps in exported resources don't point to the resources #90

Open greenwoodma opened 5 years ago

greenwoodma commented 5 years ago

If you export the resources from the ANNIE plugin and then load the xgapp file from the exported folder, it won't actually use the exported resources. This is rather confusing as it gives you no way of loading ANNIE with the resources you want to edit, other than to reconstruct the app from scratch.

The reason this happens is that we no longer record param values in the xgapp files when they are the default values specified in the creole metadata. This means we don't have to worry about version changes, or missing bug fixes where a param might change etc. This works fine when you load an app from within a plugin as it then creates a PR and gets the default values for a resource which will point at resources within the plugin. However, once you export the resources and load that app, the params are still filled in to point inside the plugin.

This could be rather confusing (I'm not the only one to get tripped up by this recently), but unfortunately I can't think of a good way of "fixing" this. It would be possible to re-write the xgapp files when they are exported from the plugin to have relative paths included, but to do so you'd need to actually load the plugin to get the default values to figure out how to re-write them, and we don't insist on a plugin being loaded for you to export the resources. In fact you might need to load the app to be able to do this correctly, which could cause other plugins to be loaded etc.

It's something I think would be nice to fix, but I'm currently at a loss to know how exactly.

johann-petrak commented 4 years ago

What is the biggest advantage of not storing the actual value used, i.e. storing the default value when it was used?

If a default value changes between plugin versions, then with the current implementation it may change the behavior of the plugin while if the original value is stored, it may not change it (still could because of code changes between versions, of course). So the ability to fix parameter-related bugs is a bit of a double-edged sword here. But there are probably other, more important reasons not to store the default parameter value used in the xgapp? On the other hand if storing the actual value used fixes the resource exported from JAR problem that would be a significant advantage.

greenwoodma commented 4 years ago

Yes, one of the reasons for not storing the default values was so that on upgrade we could guarantee the new plugin version would work as intended. It also makes working on the plugins themselves easier as the xgapp files tend to stay in sync more with the code because you don't need to worry about ensuring they are using the right values etc. (I found a number of xgapps when we moved to maven that were subtly broken this way).

To be honest I'm not sure how much of a bug this actually is. I think it's only ever confused two people and only briefly. It's never been raised on the mailing list, for instance. As I said in the initial report it's the kind of thing that would be nice to fix but I don't thinking it's a blocker on a new release. Certainly the benefits of the current approach far outweigh storing defaults in the xgapp within the plugin itself.

johann-petrak commented 4 years ago

I am not sure I can see that: if a plugin default parameter changes, the chances that keeping the old parameter for an existing pipeline is better are 50-50 because it depends on the semantics of the change. A change of the default value could just as easily break the correct functioning of the plugin without any warning, depends on the change. If the plugin even changed the parameter name then the current situation would help if the new parameter also has a default value, but again, the cleaner solution would have been to deprecate the old parameter and still support it via a hidden parameter for a version or two.

On the other hand, I found that the ability to export a pre-configured pipeline and adapt it is a major benefit and something that could make working with GATE a lot easier in many situations where we provide ready-made pipelines with a plugin. If that does not really work as expected, then I would disagree about benefits.

Of course a totally different way to look at this is this: the reason why it is especially disappointing that exporting the pipeline does not use exported resources is that it is currently impossible to easily change an init parameter. One could easily just update one runtime parameter after the other as needed, but with init parms it would involve removing the PR and creating a new one from scratch with is annoying and error-prone. So something that would allow to recreate a PR from an existing one by pre-filling in the init parms from the existing PR, setting the runtime parms from the old PR and splicing the new PR into where the old PR was would help to simplify that I think.

greenwoodma commented 4 years ago

But putting the defaults into the xgapp wouldn't help with changing an init param, as the default value is the full creole URI containing the plugin version etc. so still wouldn't load the exported resources. What we would need to do is work out the default value and then rewrite it to a relative URL based on where you export the resources. As I said in the original bug description this isn't so easy, hence why it's not been fixed yet.

I agree though, making it work would certainly make life easier in a number of situations where you want to make use of an existing xgapp that was bundled in a plugin, so it's certainly worth fixing at some point.

johann-petrak commented 4 years ago

Yes you are right, this is why I thought maybe dealing with the issue from another side would be easier: just accept that the exported gapp may point to the original resources in the jar (and maybe notify the user about that when exporting) and if you want to change that, make it easy to make it point to other resources. This can be done immediately with runtime parameters but we would need some way to do it easily with init parms. Then this issue would be a wontfix :)

greenwoodma commented 4 years ago

I suppose if we made the init param viewer an editor where you had to click save and reinitialise (a bit like the gaz editor) then that would make it easy to change the init params and as you say make this (mostly) a none issue. Only wriggle, what happens if you change and reinit something that's been duplicated.... or is that something we should ignore in this case?

johann-petrak commented 4 years ago

I think duplication is not an issue in the GUI at all for now. Should we ever want to allow multiprocessing in the GUI, we need to think about how to do that properly anyway, e.g. maybe make duplicated instances live only for as long as the multiprocessing executor runs the pipeline (otherwise the duplicates may multiply out of control anyway)? I like the idea that we could maybe implement this directly in the init param viewer.