PTV-1315 Fix AlignmentSet viewer #2501
Conversation
This is a partial fix, and forms the basis for the missing reads set viewer May form the basis of additional imrovements to set viewers.
…ouldn't get it to work, will try again so leaving in place for now.
… build w/ nodejs14 since GHA uses that by default (new lock file). (should specify engine in GHA workflows if want 16.) Also adds stylelint (since used at github for validating css) and basic stylelint config.
|
This pull request fixes 1 alert when merging 0bfc5a9 into 8769a64 - view on LGTM.com fixed alerts:
|
…ly installed Jupyter version. We need a library installed for testing, and it appears to be fine to have the global react and the amd-module react at the same time.
|
This pull request introduces 9 alerts and fixes 1 when merging cd58cf5 into 8769a64 - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 9 alerts and fixes 1 when merging cff7f08 into 8769a64 - view on LGTM.com new alerts:
fixed alerts:
|
|
This pull request introduces 9 alerts and fixes 1 when merging abd0057 into 8769a64 - view on LGTM.com new alerts:
fixed alerts:
|
Makefile
Outdated
| @@ -26,6 +26,9 @@ run-dev-image: | |||
| install: | |||
| bash $(INSTALLER) | |||
|
|
|||
| install-node-modules: | |||
There was a problem hiding this comment.
This is a bit misleadingly named as it actually moves kbase node modules elsewhere; it doesn't install npm modules if they aren't already there. Can you rename it something more appropriate?
There was a problem hiding this comment.
Sure. Had hoped it was clear that the "-node-modules" suffix implies that it installs stuff from "node_modules", rather than something like "-node-packages", which nom install does, but alas clearly not, and punning off of make install, which since it "installs" node modules into the narrative.
So, brand-new name: "copy-node-modules-into-ext-modules" captures that it is a "copy", that the subject is the "node modules" directory tree, and that they are being copied into the "ext_modules" directory, which is where node package UMD modules and related assets are installed, er, copied for usage in the front end.
| // setType: null, | ||
| // error: null |
| // return e(this.state.module, { | ||
| // token: this.props.token, | ||
| // workspaceURL: this.props.workspaceURL, | ||
| // serviceWizardURL: this.props.serviceWizardURL, | ||
| // objectRef: this.props.objectRef | ||
| // }); |
| className: 'alert alert-danger' | ||
| }, [ | ||
| 'Error: ', | ||
| message |
There was a problem hiding this comment.
you could remove both the elses and have the default text for message be Unknown Error -- save a bit of space and repetition
| @@ -0,0 +1,55 @@ | |||
| .SetBrowser .Table { | |||
There was a problem hiding this comment.
Could you move all of the css files (including those that were in the other samples PR) into the css folder so that it'll be easy to convert them all to scss when that glorious day arrives when we may finally merge the truss branch into develop?
|
|
||
| renderOverview() { | ||
| const isLoading = [null, 'loading'].includes(this.props.set.selectedItem.status); | ||
| // const isLoading = true; |
| if (!props.method) { | ||
| console.error('method not supplied to Loader'); | ||
| this.state = { | ||
| status: 'error', | ||
| error: new Error('Prop "method" required for Loader Component') | ||
| }; | ||
| return; | ||
| } | ||
|
|
||
| if (!props.module) { | ||
| console.error('module not supplied to Loader Component'); | ||
| this.state = { | ||
| status: 'error', | ||
| error: new Error('Prop "method" required for Loader Component') | ||
| }; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Combine these to use a single method. Note the error has the wrong text in the second case.
There was a problem hiding this comment.
this code is long gone now, so no worries.
|
This pull request fixes 2 alerts when merging a819699 into 8769a64 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 2 alerts when merging 43ee556 into 8769a64 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 2 alerts when merging c241c8a into 8769a64 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging d56e40e into 12434f6 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging 9ea17c4 into 12434f6 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging d1d796f into 12434f6 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging a8a0d79 into b593d07 - view on LGTM.com fixed alerts:
|
…ge and esp post merge-conflict resolution, and there are some out of date packages to boot.
|
This pull request fixes 1 alert when merging b07e84d into b593d07 - view on LGTM.com fixed alerts:
|
…s during testing; functionally equivalent otherwise.
|
This pull request fixes 1 alert when merging 152e8a6 into b593d07 - view on LGTM.com fixed alerts:
|
|
This pull request fixes 1 alert when merging 4ce8d52 into b593d07 - view on LGTM.com fixed alerts:
|
|
Kudos, SonarCloud Quality Gate passed!
|
|
This pull request fixes 1 alert when merging a904655 into b593d07 - view on LGTM.com fixed alerts:
|
DRAFT
Please do review this, modulo the fixes for list_objects, but wait on merging and a quick re-review after the official list_objects fix is in place (after removing the fix in this PR and merging in develop branch with the fix.)
Taking this out of draft mode as the workflow triggered by develop branch PRs omits build and push to GHCR if the PR is in draft mode.
We could change the workflow to ignore draft status, unless there is a good reason not to.
Using draft status to build and deploy images allows iteration on a set of changes which can be deployed for review with the change instigators (issue reporter, product owner, etc.).
Or a similar workflow could be added for feature-* and fix-* branches.
Description of PR purpose/changes
This fix addresses PTV-1315, which was a response to a public ticket which reported that a user was unable to adequately access the elements of an RNASeq reads alignment set. It ends up the problem was because the viewer responsible, kbaseGenericSetViewer, was never fully implemented and generated an empty display. Since the user who originally reported the error stopped responding to the public ticket eventually, and the ticket was closed, this fix should not be strictly interpreted as solving their specific issue, but the issue any user would encounter when attempting to view an object which would be viewed with this viewer.
Although this set of changes may seem large, it is focused on a single widget, kbaseGenericSetViewer.
This widget is supposed to handle objects of the various set types in the KBaseSets workspace type module. However, it was never fully implemented.
This set of changes adds implementations for
KBaseSets.ReadsAlignmentSetandKBaseSets.AssemblySet. Of the other 6 types in the KBaseSets module, 4 are implemented in other viewers, and two do not have any viewer.When initially doing the work for, the viewer was reimiplemented in preact, since preact had already been introduced to Jupyter notebook and presumably would be the basis for future work, it is a better platform on which to build new ui code, and the fact that the original code needed repair even to be usable as a basis for further work.
However, the original PR was held off since preact was then going to be replaced with React in a future update. When I revisited the PR, I found that indeed preact had been replaced by React, and I proceeded to port the work from preact to React.
Viewer changes
The main viewer entrypoint is
kbaseGenericSetViewer.js, a traditionalkbwidgetbased widget. This widget acts as an entrypoint for the top level React component.The React componets are divided into "loaders" and "viewers". A loader is like a controller, it is responsible for interacting with KBase service apis - fetching the data, showing loading spinners, and displaying error messages for errors encountered. The viewers are then free to simply display the fetched data.
The top level
SetLoaderis responsible for determining the object type, and calling the appropriateSetAPImethod, based on the type. The helper moduleSetTypeResolverprovides a small configuration object given a supported versionless type id. TheSetViewerdisplays summary information about the set, and provides a select control with which the user can select the set element to inspect. By default, the first set element is displayed.The SetLoader invokes the
SetElementLoaderto display a set element. The element loader determines the appropriate type-specific set element viewer to invoke, again based on resolution by SetTypeResolver. The element viewers are located in theSetElementsdirectory.React support changes
Although Jupyter contains react support, I discovered it is very limited. The react and react-dom libraries are available only through globally mounted instances which are loaded in the main html template
page.html. No AMD modules are available.Initially I attempted to use the global instances with module wrappers. That is pretty easy and it works. However, this thwarts testing, since the tests do not have access to the global instances.
So it is necessary to install react and react-dom node modules via npm, to copy their umd-packaged files into the codebase, and to add them to the requirejs configuration in
narrative_paths.js. Once there, it is better to use them for both runtime and testing - then we can ensure that we are testing against the same library.The only worry here is issues intruduced by having a globally-loaded react and an amd-loaded react. Not ideal, but it does not seem to be a problem. In addition, the Jupyter global react is only used in one location that I can find, and we do not use that feature, further reducing any risk.
I also added
prop-typeswhile finishing up, since it obviates the need for documenting the structure of component props, and also serves a purpose in validating component props.A caveate of React usage in the Narrative in general is that the programming style is devoid of JSX. This style of React programming is less familiar than JSX, but still carries most of the React advantages. It might be advantageous to use the
htmlibrary to allow usage of JSX-like syntax in template strings. This technique allows a more familiar programming structure, and transformaing to actual JSX in the future is relatively trivial.Regarding viewer coverage of KBaseSets
(note - this section ripped out of widets/function_output/KBaseSets/README.md)
The original intention of this viewer was to support all of the types implemented in KBaseSets. That work has not yet been completed:
Also, see: https://github.com/kbase/NarrativeViewers/blob/dd1eeeba0ba1faacd6c3596a9413109aeb82e32e/ui/narrative/methods/view_generic_set/spec.json#L21
Below is the status of each KBaseSet type:
Implemented in this PR:
type: KBaseSets.ReadsAlignmentSet
SetAPI method: SetAPI.get_reads_alignment_set_v1
type: KBaseSets.AssemblySet
SetAPI method: SetAPI.get_assembly_set_v1
Implemented in other viewers:
type: KBaseSets.DifferentialExpressionMatrixSet
SetAPI method: SetAPI.get_differential_expression_matrix_set_v1
Implemented by:
https://github.com/kbase/NarrativeViewers/tree/master/ui/narrative/methods/view_differential_expression_matrix_set
and this viewer does not work
type: KBaseSets.ExpressionSet
SetAPI method: SetAPI.get_expression_set_v1
Implemented by:
https://github.com/kbase/NarrativeViewers/tree/master/ui/narrative/methods/view_rnaseq_sample_expression
and that viewer doesn't work
type: KBaseSets.ReadsSet
SetAPI method: SetAPI.get_reads_set_v1
Implemented by:
https://github.com/kbase/NarrativeViewers/tree/master/ui/narrative/methods/view_reads_set
and the current viewer works
type: KBaseSets.SampleSet
SetAPI method: SetAPI.sample_set_to_samples_info (I think)
Implemented by:
https://github.com/kbase/NarrativeViewers/tree/master/ui/narrative/methods/view_sample_set
but note that this viewer does not utilize the SetAPI, which was implemented against the search, which has never been fully implemented for search. Also Samples and RNASeqSamples seem to be conflated a bit in SetAPI.
Not implemented anywhere:
type: KBaseSets.FeatureSetSet
SetAPI method: SetAPI.get_feature_set_set_v1
can't find any data, or any app which outputs this type, or accepts as input (according to the app browser)
type: KBaseSets.GenomeSet
SetAPI.get_genome_set_v1
Can't find a way to get ahold of a KBaseSets.GenomeSet to play with.
Jira Ticket / Issue
e.g. https://kbase-jira.atlassian.net/browse/DATAUP-X
DATAUP-69 Adds a PR template)Testing Instructions
Dev Checklist:
Updating Version and Release Notes (if applicable)