adds support for registering descriptions and example imports for artifact classes #655

gregcaporaso · 2022-10-18T20:18:56Z

Adds support for registering descriptions and import examples for artifact classes. Artifact class is a new term that we're using to refer to semantic types with a registered directory format. Previously these would be registered with Plugin.register_semantic_type_to_format, and that is still possible for backward compatibility. These can also now be registered with Plugin.register_artifact_class, which optionally takes a description and a dict of usage examples.

Addresses #649.

…tion

ArtifactClassRecords are now returned by PluginManager.get_semantic_types() as these are more descriptive than SemanticTypeRecords. For backward compatibility, ArtifactClassRecords have values semantic_type and type_expression, which are set to the same value upon creation of ArtifactClassRecords during plugin registration.

Plugin.types returns dict of ArtifactClassRecords instead of SemanticTypeRecords as the former are more descriptive.

Plugin.type_formats was renamed Plugin.artifact_classes for clarity, and a Plugin.type_formats property was added for backward compatibility. PluginManager.type_formats was renamed PluginManager.artifact_classes for clarity, and a PluginManager.type_formats property was added for backward compatibility. Both properties return the corresponding artifact_classes variable

ebolyen

Thanks @gregcaporaso, this all looks good to me, but one thing that I think will simplify a lot of the code would be to turn artifact_classes into a dictionary keyed by semantic type.

Then anywhere you cared about a list, artifact_classes.values() would be fine, and the rest of the time, you actually are interested in the semantic type for duplicate checking which would be a key.

ebolyen · 2022-10-21T02:01:29Z

qiime2/core/testing/plugin.py

+
+    to_import = use.init_format('to_import', factory, ext='.hello')
+
+    ints = use.import_from_format('ints',


The linter is upset about these, you can delete the variable assignment and the usage example won't care

ebolyen · 2022-10-21T02:02:38Z

qiime2/core/testing/plugin.py

+    def factory():
+        from qiime2.core.testing.format import IntSequenceFormat
+        from qiime2.plugin.util import transform
+        ff = transform([1, 2, 3], to_type=IntSequenceFormat)


This is probably the best use of transform within a plugin context I've seen. I always forget this function exists.

ebolyen · 2022-10-21T02:04:25Z

qiime2/plugin/plugin.py

+    def type_formats(self):
+        # self.type_formats was replaced with self.artifact_classes - this
+        # property provides backward compatibility
+        return self.artifact_classes


ebolyen · 2022-10-21T02:12:54Z

qiime2/plugin/plugin.py

+            examples = {}
+
+        registered_artifact_classes = \
+            set(str(e.type_expression) for e in self.artifact_classes)


This may be worth storing on the plugin instance as a private member, as otherwise we'll be constructing this set for each registration, resulting in n^2 iterations through self.artifact_classes.

A good option might be to turn types into a mappingproxy of an internal dict, then you can simply check against self.types as things are being constructed.

actually instead of self.types self.artifact_classes would be fine I think.

ebolyen · 2022-10-21T02:14:31Z

qiime2/plugin/plugin.py

@@ -79,7 +83,7 @@ def __init__(self, name, version, website, package=None, project_name=None,
        self.views = {}
        self.type_fragments = {}
        self.transformers = {}
-        self.type_formats = []
+        self.artifact_classes = []


This might be better as a dictionary for some of the registration behavior

Good call - thanks for catching that! Made this change and most of my commit responding to your comments is updates to address this.

ebolyen · 2022-10-21T02:20:38Z

qiime2/sdk/plugin_manager.py

@@ -201,7 +201,7 @@ def _integrate_plugin(self, plugin):
                )

            self.formats[name] = record
-        self.type_formats.extend(plugin.type_formats)
+        self.artifact_classes.extend(plugin.artifact_classes)


This was missing originally, but we should do a similar check as the other properties to make sure duplicate registrations (across plugins) are forbidden.

I added this test and validated that it works manually and reports a useful error message (from my test: ValueError: Duplicate semantic type ('PCoAResults') defined in plugins: 'sapienns' and 'types'). I didn't see where/if this type of functionality was being unit tested, so I didn't add unit tests for it now.

There's no pleasant way to test for this, so eyeballing it is fine with me.

ebolyen · 2022-10-25T18:58:53Z

qiime2/sdk/plugin_manager.py

+        # Evan, ideally we could just lookup semantic_type in
+        # self.artifact_classes but properties get in the way. Is there a way
+        # to strip properties so this could be simplified to return
+        # self.artifact_classes[semantic_type] while catching a KeyError?


There's not a global helper for this, but there is a .duplicate() method which allows you to override attributes of a type, so (SomeType % Foo).duplicate(predicate=IntersectionExp()) would accomplish your goal. It's not pretty and it's also not recursive, so any variant fields would need to be recursively duplicated as well.

The behavior below is correct and the above sounds like too much work. So let's ignore this for now.

ebolyen

Just promoting the comment to a TODO, since it's useful context and asks the kind of question which would necessitate some nicer mechanisms than what I described.

qiime2/sdk/plugin_manager.py

ebolyen · 2022-10-25T19:04:01Z

Thanks @gregcaporaso!

gregcaporaso · 2022-10-25T19:12:06Z

Thanks for the reviews @ebolyen!

gregcaporaso added 16 commits October 3, 2022 09:42

defines artifact class registration including description and examples

6ad9bf9

add unit tests

2f7abe3

squash

5cef1ca

add method supporting lookup of type_formats

c48415e

tests of PluginManager.get_artifact_classes()

091e3a9

rename artifact_format as directory_format on artifact class registra…

c9ddb3c

…tion

delete unreachable code

1a9e7ab

changed type of examples

b3d43d6

updates check for non-concrete semantic types

1cdbf54

register artifact classes as concrete types rather than type expressions

acae77a

BUG: disallow duplicate artifact class registration

052d5c4

update return type of Plugin.types

dfe3510

Plugin.types returns dict of ArtifactClassRecords instead of SemanticTypeRecords as the former are more descriptive.

create and add usage examples to test

2d3a425

adds usage examples and tests, and linting

eb71290

gregcaporaso mentioned this pull request Oct 18, 2022

updates to artifact class registration qiime2/q2-types#283

Merged

gregcaporaso requested a review from ebolyen October 18, 2022 20:29

ebolyen requested changes Oct 21, 2022

View reviewed changes

address review comments

ebebe7f

gregcaporaso marked this pull request as ready for review October 24, 2022 01:43

gregcaporaso requested a review from ebolyen October 24, 2022 01:43

update to fix failing q2-types tests

600731b

ebolyen reviewed Oct 25, 2022

View reviewed changes

qiime2/sdk/plugin_manager.py Outdated Show resolved Hide resolved

Update qiime2/sdk/plugin_manager.py

878bf42

ebolyen merged commit 05c515b into qiime2:master Oct 25, 2022

gregcaporaso deleted the issue-649 branch October 25, 2022 19:12

gregcaporaso mentioned this pull request Dec 2, 2022

add usage examples to artifact classes qiime2/q2-types#288

Open

ebolyen mentioned this pull request Feb 24, 2023

enable registered descriptions of semantic types and formats #649

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds support for registering descriptions and example imports for artifact classes #655

adds support for registering descriptions and example imports for artifact classes #655

gregcaporaso commented Oct 18, 2022

ebolyen left a comment

ebolyen Oct 21, 2022

ebolyen Oct 21, 2022

ebolyen Oct 21, 2022

ebolyen Oct 21, 2022

ebolyen Oct 21, 2022

ebolyen Oct 21, 2022

gregcaporaso Oct 24, 2022

ebolyen Oct 21, 2022

gregcaporaso Oct 24, 2022 •

edited

Loading

ebolyen Oct 25, 2022

ebolyen Oct 25, 2022

ebolyen left a comment

ebolyen commented Oct 25, 2022

gregcaporaso commented Oct 25, 2022


		to_import = use.init_format('to_import', factory, ext='.hello')

		ints = use.import_from_format('ints',

adds support for registering descriptions and example imports for artifact classes #655

adds support for registering descriptions and example imports for artifact classes #655

Conversation

gregcaporaso commented Oct 18, 2022

ebolyen left a 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

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

gregcaporaso Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebolyen left a comment

Choose a reason for hiding this comment

ebolyen commented Oct 25, 2022

gregcaporaso commented Oct 25, 2022

gregcaporaso Oct 24, 2022 •

edited

Loading