search

GEGlobalResearch / DARPA-ASKE-TA1

ANSWER Project to demonstrate knowledge-driven extraction of scientific models from code and texts
Other
8 stars 5 forks source link

renaming of SadlResources in SADL and Dialog #104

Open crapo opened 4 years ago

crapo commented 4 years ago

@kittaakos , one of the capabilities of xtext that can be implemented for a DSL is, I believe, the abiltiyh to rename a concept and have the new name appear wherever it is referenced. How hard would it be to implement this for SadlResources in SADL and Dialog? It would be very helpful for this project and in general. Is this something that could be done in a couple of days? If so, how soon could you do it?

kittaakos commented 4 years ago

It should work out of the box. I wanted to verify this, but I could not build it. I am on MissingPatterns in SADL and on the master branch in Dialog. Do you know what's causing the problem?

[INFO] Scanning generated classes for implementations...
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO] 
[INFO] DARPA-ASKE ......................................... SUCCESS [  0.002 s]
[INFO] DARPA-ASKE Dependencies ............................ SUCCESS [  0.624 s]
[INFO] DARPA-ASKE Core .................................... FAILURE [ 34.575 s]
[INFO] DARPA-ASKE IDE ..................................... SKIPPED
[INFO] DARPA-ASKE UI ...................................... SKIPPED
[INFO] DARPA-ASKE Tests ................................... SKIPPED
[INFO] DARPA-ASKE UI Tests ................................ SKIPPED
[INFO] DARPA-ASKE Eclipse Feature ......................... SKIPPED
[INFO] DARPA-ASKE p2 Update Site .......................... SKIPPED
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 03:31 min
[INFO] Finished at: 2020-04-15T16:42:37+02:00
[INFO] Final Memory: 146M/1682M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal eu.somatik.serviceloader-maven-plugin:serviceloader-maven-plugin:1.1.0:generate (default) on project com.ge.research.sadl.darpa.aske.dialog: Execution default of goal eu.somatik.serviceloader-maven-plugin:serviceloader-maven-plugin:1.1.0:generate failed: An API incompatibility was encountered while executing eu.somatik.serviceloader-maven-plugin:serviceloader-maven-plugin:1.1.0:generate: java.lang.IncompatibleClassChangeError: com.ge.research.sadl.darpa.aske.parser.antlr.internal.InternalDialogParser and com.ge.research.sadl.darpa.aske.parser.antlr.internal.InternalDialogParser$DFA28 disagree on InnerClasses attribute
[ERROR] -----------------------------------------------------
[ERROR] realm =    plugin>eu.somatik.serviceloader-maven-plugin:serviceloader-maven-plugin:1.1.0
[ERROR] strategy = org.codehaus.plexus.classworlds.strategy.SelfFirstStrategy
[ERROR] urls[0] = file:/Users/akos.kitta/.m2/repository/eu/somatik/serviceloader-maven-plugin/serviceloader-maven-plugin/1.1.0/serviceloader-maven-plugin-1.1.0.jar
[ERROR] urls[1] = file:/Users/akos.kitta/.m2/repository/org/sonatype/plexus/plexus-build-api/0.0.7/plexus-build-api-0.0.7.jar
[ERROR] urls[2] = file:/Users/akos.kitta/.m2/repository/javax/enterprise/cdi-api/1.0/cdi-api-1.0.jar
[ERROR] urls[3] = file:/Users/akos.kitta/.m2/repository/com/google/guava/guava/10.0.1/guava-10.0.1.jar
[ERROR] urls[4] = file:/Users/akos.kitta/.m2/repository/com/google/code/findbugs/jsr305/1.3.9/jsr305-1.3.9.jar
[ERROR] urls[5] = file:/Users/akos.kitta/.m2/repository/org/sonatype/sisu/sisu-guice/3.1.0/sisu-guice-3.1.0-no_aop.jar
[ERROR] urls[6] = file:/Users/akos.kitta/.m2/repository/aopalliance/aopalliance/1.0/aopalliance-1.0.jar
[ERROR] urls[7] = file:/Users/akos.kitta/.m2/repository/org/eclipse/sisu/org.eclipse.sisu.inject/0.0.0.M5/org.eclipse.sisu.inject-0.0.0.M5.jar
[ERROR] urls[8] = file:/Users/akos.kitta/.m2/repository/org/codehaus/plexus/plexus-component-annotations/1.5.5/plexus-component-annotations-1.5.5.jar
[ERROR] urls[9] = file:/Users/akos.kitta/.m2/repository/backport-util-concurrent/backport-util-concurrent/3.1/backport-util-concurrent-3.1.jar
[ERROR] urls[10] = file:/Users/akos.kitta/.m2/repository/org/codehaus/plexus/plexus-interpolation/1.11/plexus-interpolation-1.11.jar
[ERROR] urls[11] = file:/Users/akos.kitta/.m2/repository/junit/junit/3.8.1/junit-3.8.1.jar
[ERROR] urls[12] = file:/Users/akos.kitta/.m2/repository/org/codehaus/plexus/plexus-utils/3.0.22/plexus-utils-3.0.22.jar
[ERROR] urls[13] = file:/Users/akos.kitta/.m2/repository/org/codehaus/plexus/plexus-compiler-api/2.5/plexus-compiler-api-2.5.jar
[ERROR] Number of foreign imports: 1
[ERROR] import: Entry[import  from realm ClassRealm[project>com.ge.research.sadl.darpa.aske.dialog:com.ge.research.sadl.darpa.aske.dialog.parent:1.0.0-SNAPSHOT, parent: ClassRealm[maven.api, parent: null]]]
[ERROR] 
[ERROR] -----------------------------------------------------
[ERROR] 
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/PluginContainerException
[ERROR] 
[ERROR] After correcting the problems, you can resume the build with the command
[ERROR]   mvn <goals> -rf :com.ge.research.sadl.darpa.aske.dialog
akos.kitta@Akoss-MacBook-Pro com.ge.research.sadl.darpa.aske.dialog.parent %
crapo commented 4 years ago

sorry to be imprecise. ASKE TA1 branch should be CodeExtraction for this.

kittaakos commented 4 years ago

I am on that branch, but I still have the same build error. Were there any dependency updates recently?

crapo commented 4 years ago

I have an updated target file for sadlos2 MissingPatterns that I haven't pushed (and I got from you I think) because Alfredo isn't yet on Eclipse 2020-03. Otherwise none that I know of. I don't think I've seen this error.

crapo commented 4 years ago

I have tested Rename element and can report that it doesn't not work.

kittaakos commented 4 years ago

Otherwise none that I know of. I don't think I've seen this error.

Seem to be OK now, I had to wipe the entire repository state locally.

It would be very helpful for this project and in general.

I agree. 👍 It's a good idea.

Is this something that could be done in a couple of days?

I have to check why it does not work out of the box, but I think it should be feasible.

If so, how soon could you do it?

I can start working on this tomorrow, but let me check and get back to you with better time-estimation.

crapo commented 4 years ago

Thanks!!

kittaakos commented 4 years ago

but let me check and get back to you with better time-estimation.

@crapo, it is a non-trivial task. Xtext supports the rename/refactoring in two different ways:

I am going to start the implementation using the latter approach. I will let you know if I run into some unexpected issues, but I still believe it's feasible.

crapo commented 4 years ago

Thank you. Please keep me posted. This would be a significant improvement to SADL usability.

kittaakos commented 4 years ago

Short heads-up: we cannot reuse anything as-is from the Xtext for the renaming. We have to calculate the edit locations manually. I've managed to make some progress, but there is still a lot to code.

screencast 2020-04-16 17-27-02

Renaming if the declaration is in another file, could be problematic; I will continue with that part to see if there are any show stoppers as soon as possible.

crapo commented 4 years ago

Thank you!

crapo commented 4 years ago

As we move towards our final demo, this renaming would be extremely helpful, even if it only worked within a single .dialog file by the demo freeze, which will be by Wednesday next week. Let me know if focusing on that helps. I judge from the animated image that you may already have that working in some context. Would it work in .dialog files?

kittaakos commented 4 years ago

@crapo, I made some progress but I am not as optimistic as I was yesterday. We can support the followings:

Rename local SADL resource in both SADL and Dialog file: screencast 2020-04-17 16-26-31

Rename SADL resource in both SADL and Dialog file, will update SADL resource at declarations-side: screencast 2020-04-17 16-30-13

Issues (I am aware of so far):

crapo commented 4 years ago

Not sure if this would make it any easier, but it would be acceptable to have to go to the definition to rename. I don't understand the last bullet. The primary purpose would be that we extract concepts from code comments that are suggested additions to the ontology, and these appear as augmented (semantic) types in the equation signature. We would like the user to be able to rename these concepts (where they are defined in the same dialog file) and have them also be renamed in the augmented type.

kittaakos commented 4 years ago

I created two PRs if you want to experiment with the changes. Nothing else should be affected:

SADL PR against the MissingPatterns branch: https://github.com/crapo/sadlos2/pull/416 Dialog PR against the CodeExtraction branch: https://github.com/GEGlobalResearch/DARPA-ASKE-TA1/pull/105

crapo commented 4 years ago

I hate to show my git ignorance, but how do I experiment without merging the PRs? Just go to the branches that you must have created?

kittaakos commented 4 years ago

Just go to the branches that you must have created?

Yes.

In a command line, you fetch (you can pull too), and checkout the branch.

kittaakos commented 4 years ago

  • We should not allow renaming built-ins,

  • We should not allow renaming externals,

  • If a SADL resource is defined in a SADL file AND used in more than one Dialog file, for some reason the rename does not work in the Dialog (I have to investigate it),

I have updated both PRs; the above items are fixed now. 👆

crapo commented 4 years ago

Do I see correctly that you've merged these into MissingPatterns?

kittaakos commented 4 years ago

Do I see correctly that you've merged these into MissingPatterns?

Not at all. The changes are still on the GH-415 branch. Have you had the chance to try it?

kittaakos commented 4 years ago
  • We have zero tests,

I have added some basic testing for the rename, but the Dialog content updates break it in some cases; please reference the screencast:

screencast 2020-04-20 15-39-48

The editor content is updated with:

CM: Concept x is not defined; please define or do extraction.

And it breaks the refactoring workflow.

crapo commented 4 years ago

Humm... this is a little like the discussion about JenaBasedDialogModelProcessor onValidate. The call to AnswerCurationManager processConversation shouldn't happen until the renaming is complete and the new name is resolved. I think I can add logic to detect an invalid query. Would onValidate get called again after the imported ontology is updated and the new name is resolvable?

kittaakos commented 4 years ago

this is a little like the discussion about JenaBasedDialogModelProcessor onValidate

Yes, sort of. In general, we should not update the resource when validating it but trigger the conversation updates on-demand, with another UX feature: https://github.com/GEGlobalResearch/DARPA-ASKE-TA1/issues/68#issuecomment-589554057. I know, it's out of scope for now but please let's discuss this after the demo.

The call to AnswerCurationManager processConversation shouldn't happen until the renaming is complete and the new name is resolved.

Correct.

I think I can add logic to detect an invalid query.

That would be great!

Would onValidate get called again after the imported ontology is updated and the new name is resolvable?

Yes. It did not work before like that, but I made this happen in #105.

crapo commented 4 years ago

Unfortunately, it isn't that easy.

I think I can add logic to detect an invalid query.

That would be great!

If the concept isn't defined, we want the processConversation call to tell the user that the concept isn't defined. I would need to know that a rename workflow is in-progress in order to just not make the call to processConversation. Is that possible?

crapo commented 4 years ago

I note that renaming a concept in a Dialog editor does not turn on the dirty flag--I guess it doesn't need to be saved? Or is that unexpected?

kittaakos commented 4 years ago

@crapo, I've managed to put together a state which works with my super naive use-cases. I am going to update the PR and prepare it for the merge. Can you please try it, and if you are OK with it, you can integrate the change into the CodeExtraction branch.

Also, you will have the demo tomorrow, right? Do you want to have a quick sync-tcon? If so, please email the link, I am going to be available for another hour or so. Or if you have any other requests, I can work on them.

crapo commented 4 years ago

Tomorrow is the code freeze for the demo on Friday. When the PRs are updated I will try it out. If needed we can synch up then or tomorrow when my day starts if you are available.

kittaakos commented 4 years ago

I have updated both PRs. The changeset is ready if you want to try it again.

Note: I have force pushed to the remote branches!

crapo commented 4 years ago

@kittaakos , this is looking good! I'll do more testing tomorrow, but it seems pretty well behaved for what we want to demo.