[WIP] Refactoring and cleanup #49

Squareys · 2016-09-06T14:26:23Z

Hello everybody!

This is @dietzc's work-in-progress which I will be doing a polishing pass on.

TODOs:

Documentation
Batch together WIP commits with some description of what was done
Repair all FIXMEs
Proper Logging / Exception handling (which is part of FIXMEs)
Proper MissingValue Handling
Unit and Workflow-Tests where suitable
Split scripting away, open PR in knip-scripting and move it there.
Remove temp ScijavaTestCommand and Script directory, implement a replacement

Regards,
Jonathan.

This is a work-in-progress pull request and not meant to be merged before [WIP] is removed from the title!

Signed-off-by: Squareys <[email protected]>

Squareys · 2016-09-12T08:17:10Z

...cijava.commands/src/org/knime/scijava/commands/settings/DefaultSettingsModelTypeService.java

+    public SettingsModelType getSettingsModelTypeFor(final ModuleItem<?> item) {
+
+        // FIXME why do I need this check in case of scripts?... Why should you
+        // not?


@dietzc ? :)

I think the question here was/is why I need the item.getWidgetStyle()!=null check.

(Context independent description of the problem:)

null is default for getWidgetStyle() in DefaultMutableModuleItem, see here. It is used as ModuleItems for ScriptInfo, => more proof, and is only set if a matching key exists in the parameter annotation in the script.

In the @Parameter annotation style() is default "", though, which is why getWidgetStyle() is never null for CommandModule (also see this line ).

@ctrueden This is a slight inconsistency in Scijava. What is your oppinion about this? There seems to be no documentation about the getWidgetStyle() method possibly returning null, so I guess the widget style should probably be initialized with "" for DefaultMutableModuleItem?

This is a slight inconsistency in Scijava. What is your oppinion about this? There seems to be no documentation about the getWidgetStyle() method possibly returning null, so I guess the widget style should probably be initialized with "" for DefaultMutableModuleItem?

I agree this is inconsistent. However, it goes far beyond just widget style. Every attribute of the @Parameter annotation has non-null defaults, because Java annotations do not allow null values whatsoever. However, that is inconsistent with SJC behavior in general, where null—not ""—is used to indicate no/default value/behavior.

I suppose my preferred solution to this conundrum is to change the PluginInfo class to translate empty strings into nulls, as perverse as that seems...

Fixes the related FIXMEs. Signed-off-by: Squareys <[email protected]>

Signed-off-by: Squareys <[email protected]>

We handle the single row output using the "enableManualPush" flag and a final flush() method which pushes a row only if manual push is not enabled. Manual push is enabled for modules with a output listener parameter. Signed-off-by: Squareys <[email protected]>

If `.createInstancesOfType(PreprocessorPlugin.class);` does not produce results, the returned list will be empty, but not null. Signed-off-by: Squareys <[email protected]>

Signed-off-by: Squareys <[email protected]>

Squareys · 2016-09-12T09:49:09Z

org.knime.scijava.commands/src/org/knime/scijava/commands/module/DefaultNodeModule.java

+            module.resolveInput(entry.getKey());
+        }
+
+        // FIXME: do we need them all?


@dietzc Does it matter?

depends how costly the preprocessors are. As we've no put the preprocessing logic outside the run logic it seems to be fine to take all preprocessors. However, there are preprocessors which are really useless in case of headless execution.

Since they are only executed once per Node Execution, I doubt this will impact performance. What's your oppinion?

I think it is probably not worth the effort bisecting the preprocessors to find out which we need and which we don't. (Aka. premature opt... we can always revisit this if we see this is turning into a problem)

@dietzc Are you ok with that? Then I would remove the FIXME.

I removed the FIXME. If you object, I will add it back in.

Comment seen.

Signed-off-by: Squareys <[email protected]>

Example: input type is "MyMultiOutputListener" (my subclass of MultiOutputListener), then the check would suceed, since I can assign this type to MultiOutputListener. When I try to assign NodeModuleOutputChangedListener, it would throw an IllegalArgumentException. If we would just simply switch to testing whether the item type is assignable from MultiOutputListener, it could also be Object, which would also resolve Object inputs with our logic here, which is not intended. A simple check for the class being exactly MultiOutputListener.class is therefore suficient. Signed-off-by: Squareys <[email protected]>

dietzc · 2016-09-12T10:07:01Z

org.knime.scijava.commands/src/org/knime/scijava/commands/module/DefaultNodeModule.java

+         *            if <code>true</code>, signals that the module handles
+         *            pushing rows.
+         */
+        public void enableManualPush(final boolean b) {


uh, this is really state-driven now. can we avoid this method, e.g. by determining in the constructor if manual or not?! just asking, not saying it's nicer.

Well, since it is only called immediately after the constructor, I can put it into the constructor, makes sense :)

Signed-off-by: Squareys <[email protected]>

And fix a screw-up of mine for row push. Signed-off-by: Squareys <[email protected]>

Signed-off-by: Squareys <[email protected]>

Squareys · 2016-09-13T11:52:40Z

@dietzc I had this idea to split NodeModule and DefaultNodeModuleService into Scripting- and Command- to handle the slight differences (primarily "result" output module item atm). This could even be extended to more module info types than just ScriptInfo, CommandInfo. Possibly ImageJ Plugins?

My current attempt will not work, though, since the appropriate Service cannot be chosen depending on the module info type. Instead, I would introduce a new singleton plugin type which allows adding support for more ModuleInfo types in DefaultNodeModuleService (which will then handle those plugins). It would basically define two methods createNodeModule() and createOutputColumnSpecs() (possibly Class<? extends ModuleInfo) getModuleInfoType()).

Does that sound sane?

ctrueden · 2016-09-13T12:34:35Z

Why does KNIME need to handle the "result" output specially?

dietzc · 2016-09-13T14:09:17Z

@ctrueden each Script outputs a result String for logging errors etc. I think @gab1one and you have already discussed this issue and also solved it? The reason why we have to treat the result output in a a special way, is that in KNIME we create the output-spec of the resulting table (especially how many columns, which types etc) before we execute the node. However, the result is a special thing in scripting which is not part of the ModuleInfo we get after parsing the script. That's why we have to treat the result in a special way.

Either, we handle the result on our side or on scijava-side. I'd prefer to do it in scijava and if you have suggestions how to do it, I'm sure @Squareys can help you. It would be great if the abstraction layer we use in our code is ModuleInfo rather than having special cases for different types of ModuleInfo.

One related issue: scijava/scijava-common#225

ctrueden · 2016-09-13T14:13:28Z

Ok great, I thought he was talking about the Object result which SciJava adds to scripts with no declared outputs.

dietzc · 2016-09-13T15:04:18Z

Actually, this one might be problematic, too. Is it also added to the ScriptInfo?

Signed-off-by: Squareys <[email protected]>

dietzc · 2016-09-14T12:52:33Z

After discussing this with @ctrueden in person we figured that we still should special case the result output of a ScriptInfo if it was added by the framework. Especially, as we want to support n-to-0 nodes. However, we don't need two implementations in this case., I think a if(info instanceof ScriptInfo) is fine.

ctrueden · 2016-09-14T20:04:39Z

More specifically, I would recommend:

boolean hasSyntheticResult = info instanceof ScriptInfo &&
    ((ScriptInfo) info).isReturnValueAppended();

Signed-off-by: Squareys <[email protected]>

Squareys added enhancement 2 - in progress labels Sep 6, 2016

Squareys added this to the knip-scijava-1.0.0-beta milestone Sep 6, 2016

Squareys self-assigned this Sep 6, 2016

Refactor scijava integration

f5e223a

Squareys force-pushed the refactoring-and-cleanup branch from fa4c988 to f5e223a Compare September 6, 2016 14:42

Add javadoc to everything and cleanup.

ff8a056

Signed-off-by: Squareys <[email protected]>

Squareys reviewed Sep 12, 2016
View reviewed changes

Squareys added 6 commits September 12, 2016 11:42

Rename ConverterCacheService to KNIMEConverterService

82eeeac

Fixes the related FIXMEs. Signed-off-by: Squareys <[email protected]>

Avoid deprecated setResolved method

3fbb32d

Signed-off-by: Squareys <[email protected]>

Pre is never null

b3c4f7a

If `.createInstancesOfType(PreprocessorPlugin.class);` does not produce results, the returned list will be empty, but not null. Signed-off-by: Squareys <[email protected]>

Fix MultiOutputListener @parameters not being marked resolved

215d97f

Signed-off-by: Squareys <[email protected]>

Avoid deprecated ResourceAwareClassLoader constructor

fe6e1bb

Signed-off-by: Squareys <[email protected]>

Squareys reviewed Sep 12, 2016
View reviewed changes

Squareys added 2 commits September 12, 2016 11:55

Move per-node-execution preprocessing into private method.

fbd8bef

Signed-off-by: Squareys <[email protected]>

dietzc reviewed Sep 12, 2016
View reviewed changes

Squareys added 6 commits September 12, 2016 15:34

Rename KnimeProcessorTest to NodeModuleTest

76acbea

Signed-off-by: Squareys <[email protected]>

Avoid null checking getWidgetStyle()

8d9bfd4

Signed-off-by: Squareys <[email protected]>

More javadoc

24bc5ad

Signed-off-by: Squareys <[email protected]>

Use NodeLogger for logging in NodeModules and implement KNIMELogService

488620f

Signed-off-by: Squareys <[email protected]>

NodeModuleTest: Adapt to API changes in org.knime.scijava.commands

feeafa9

Signed-off-by: Squareys <[email protected]>

Specify 'manualPush' in constructor instead of setter.

2bd9815

Signed-off-by: Squareys <[email protected]>

Squareys force-pushed the refactoring-and-cleanup branch from ab41c9a to 2bd9815 Compare September 12, 2016 13:45

This was referenced Sep 13, 2016

Column selection widget for SciJava Nodes #22

Closed

Implement prototype nodes for each input / output combination #23

Closed

This was referenced Sep 13, 2016

Configure() method should fail when the inputspec is not compatible, related with #70 #39

Open

CI and Testing Github Integration #50

Open

Squareys added 5 commits September 13, 2016 12:23

Add tests for NodeModule and DefaultNodeModuleService

6ea385c

Signed-off-by: Squareys <[email protected]>

Add getPreferredDataType(Class) to KNIMEConverterService

71e1053

Signed-off-by: Squareys <[email protected]>

Delegate exception during notifyListener()

d2420bc

And fix a screw-up of mine for row push. Signed-off-by: Squareys <[email protected]>

Simplyfy getting output DataType for module output.

624b28f

Signed-off-by: Squareys <[email protected]>

Some Code cleanup

911fe46

Signed-off-by: Squareys <[email protected]>

Squareys mentioned this pull request Sep 13, 2016

Missing Cell Handling #45

Open

Squareys force-pushed the refactoring-and-cleanup branch from 8ac4e28 to 2a5a238 Compare September 13, 2016 11:59

Remove unused imports

4e0736e

Signed-off-by: Squareys <[email protected]>

Squareys force-pushed the refactoring-and-cleanup branch 2 times, most recently from 56398f7 to 255e9fc Compare September 26, 2016 13:03

Squareys and others added 3 commits September 26, 2016 15:03

Use better way of detecting synthetic "result" ouput

255e9fc

Signed-off-by: Squareys <[email protected]>

Merge branch 'master' into refactoring-and-cleanup

321c004

fixes

a1480f0

gab1one force-pushed the master branch from 9b6b2bb to a25ad9d Compare October 31, 2022 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Refactoring and cleanup #49

[WIP] Refactoring and cleanup #49

Squareys commented Sep 6, 2016 •

edited

Loading

Squareys Sep 12, 2016

dietzc Sep 12, 2016

Squareys Sep 12, 2016 •

edited

Loading

ctrueden Sep 12, 2016

Squareys Sep 12, 2016

dietzc Sep 12, 2016

Squareys Sep 12, 2016

Squareys Sep 13, 2016

Squareys Sep 13, 2016 •

edited by dietzc

Loading

dietzc Sep 12, 2016

Squareys Sep 12, 2016 •

edited

Loading

Squareys commented Sep 13, 2016

ctrueden commented Sep 13, 2016 via email

dietzc commented Sep 13, 2016 •

edited

Loading

ctrueden commented Sep 13, 2016 •

edited

Loading

dietzc commented Sep 13, 2016

dietzc commented Sep 14, 2016 •

edited

Loading

ctrueden commented Sep 14, 2016 •

edited

Loading

[WIP] Refactoring and cleanup #49

Are you sure you want to change the base?

[WIP] Refactoring and cleanup #49

Conversation

Squareys commented Sep 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Squareys Sep 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Squareys Sep 13, 2016 • edited by dietzc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Squareys Sep 12, 2016 • edited Loading

Choose a reason for hiding this comment

Squareys commented Sep 13, 2016

ctrueden commented Sep 13, 2016 via email

dietzc commented Sep 13, 2016 • edited Loading

ctrueden commented Sep 13, 2016 • edited Loading

dietzc commented Sep 13, 2016

dietzc commented Sep 14, 2016 • edited Loading

ctrueden commented Sep 14, 2016 • edited Loading

Squareys commented Sep 6, 2016 •

edited

Loading

Squareys Sep 12, 2016 •

edited

Loading

Squareys Sep 13, 2016 •

edited by dietzc

Loading

Squareys Sep 12, 2016 •

edited

Loading

dietzc commented Sep 13, 2016 •

edited

Loading

ctrueden commented Sep 13, 2016 •

edited

Loading

dietzc commented Sep 14, 2016 •

edited

Loading

ctrueden commented Sep 14, 2016 •

edited

Loading