ENH: Containers via entrypoints #126

hhslepicka · 2020-03-20T22:42:57Z

Description

As of now in order to add a new container we need to modify Happi source code and add it at the happi.containers module so it is found by the client.
Via discussion at #106 and many talks with @ZLLentz and @klauer we decided to experiment with the usage of entry_points.
The LCLS specific containers and item were moved to pcdsdevices under the following PR: pcdshub/pcdsdevices#402

Motivation and Context

Merging this will contribute to close #106

How Has This Been Tested?

Tested locally

Where Has This Been Documented?

Still need to be done. Will work on that after code comments

Screenshots (if appropriate):

In [1]: import happi
Questionnaire backend unavailable: No module named 'psdm_qs_cli'

In [2]: happi.containers.registry
Out[2]:
{pcdsdevices.happi.containers.Acromag,
 pcdsdevices.happi.containers.AreaDetector,
 pcdsdevices.happi.containers.Attenuator,
 pcdsdevices.happi.containers.BeamControl,
 pcdsdevices.happi.containers.Diagnostic,
 pcdsdevices.happi.containers.GateValve,
 pcdsdevices.happi.containers.IPM,
 pcdsdevices.happi.containers.LCLSItem,
 pcdsdevices.happi.containers.LODCM,
 pcdsdevices.happi.containers.Motor,
 pcdsdevices.happi.containers.MovableStand,
 pcdsdevices.happi.containers.OffsetMirror,
 pcdsdevices.happi.containers.PIM,
 pcdsdevices.happi.containers.PulsePicker,
 pcdsdevices.happi.containers.Slits,
 pcdsdevices.happi.containers.Stopper,
 pcdsdevices.happi.containers.Trigger,
 pcdsdevices.happi.containers.Vacuum}

In [3]: happi.containers.Acromag
Out[3]: pcdsdevices.happi.containers.Acromag

In [4]: dir(happi.containers)
Out[4]:
['Acromag',
 'AreaDetector',
 'Attenuator',
 'BeamControl',
 'Diagnostic',
 'GateValve',
 'HappiItem',
 'IPM',
 'LCLSItem',
 'LODCM',
 'Motor',
 'MovableStand',
 'OffsetMirror',
 'PIM',
 'PulsePicker',
 'Slits',
 'Stopper',
 'Trigger',
 'Vacuum',
 '__builtins__',
 '__cached__',
 '__doc__',
 '__file__',
 '__loader__',
 '__name__',
 '__package__',
 '__spec__',
 '_entries',
 'entry',
 'entrypoints',
 'inspect',
 'logger',
 'logging',
 'obj',
 'registry']

ENH: Allow containers via entrypoint happi.containers.

klauer

Exciting to see this happening... a few thoughts below

happi/containers.py

klauer · 2020-03-20T22:53:04Z

happi/containers.py

+                         # Avoid happi internal classes due to imports
+                         and not var.__module__.startswith('happi.')])
+
+        locals().update({c.__name__: c for c in registry})


Indentation right here (seems like this should happen at the end after the registry is built)?

I think you're trying to get happi.containers.<classname> working: globals() seems more correct but may do the same here. We're also risking name collisions in the namespace as this strips off the __module__ prefix, hmm...

Indentation solved with 5c39ffb. Sorry that I forgot to push before opening the PR.

About the globals()... I'm not sure.

ok, now I understand... that is correct, we inject the class name into the happi.containers namespace it would fail if we have 2 containers with the same name but different full qualified name.

klauer · 2020-03-20T22:55:04Z

happi/containers.py

+
+_entries = entrypoints.get_group_all('happi.containers')
+
+registry = set()


I wonder if the registry should be keyed as follows: registry['(module).(classname)'] = obj?

What about the name from the entry_points? This way we can make it uniform to always be the package that originated it and we guarantee unique names per package?
E.g.:
pcdsdevices.Acromag as the container and key at registry.

Hmm, maybe... but what about multi-level imports?

Would (e.g.) pcdsdevices.motors.Acromag become pcdsdevices.Acromag?

The second one will be an internal key for Happi at the registry and it will point to the proper class pcdsdevices.happi.containers.Acromag and queried. I decided to make it shorter as a convenience for users when selecting a class at the CLI but we can also make the namespace shorter at the pcdsdevices side and improve the display of the possibilities at the CLI.

In our example, the name you'd put in a happi entry to get the Acromag instantiated would be pcdsdevices.Acromag, right?

That depends but I can see the confusion. Would it be okay if I make the namespace be pcdsdevices.happi.Acromag and I switch to use the full class at the registry like you suggested before?

I'm trying to get at the heart of the problem and don't really know the "right" answer just yet to say yes or no...

full_module.classname is unambiguous. Entry_points name gives more flexibility, but adds indirection to get to the class itself.

If we populate the registry with both entry_point_name.classname and module.classname, I can imagine that'd get confusing as well. I think it has to be one or the other.

The entrypoint name would allow for us to move around classes and refactor codebases without updating our device databases. Maybe that's preferable?

As of now we have:

In [1]: import happi Questionnaire backend unavailable: No module named 'psdm_qs_cli' In [2]: happi.containers.registry Out[2]: {'pcdsdevices.Acromag': pcdsdevices.happi.containers.Acromag, 'pcdsdevices.AreaDetector': pcdsdevices.happi.containers.AreaDetector, 'pcdsdevices.Attenuator': pcdsdevices.happi.containers.Attenuator, 'pcdsdevices.BeamControl': pcdsdevices.happi.containers.BeamControl, 'pcdsdevices.Diagnostic': pcdsdevices.happi.containers.Diagnostic, 'pcdsdevices.GateValve': pcdsdevices.happi.containers.GateValve, 'pcdsdevices.IPM': pcdsdevices.happi.containers.IPM, 'pcdsdevices.LCLSItem': pcdsdevices.happi.containers.LCLSItem, 'pcdsdevices.LODCM': pcdsdevices.happi.containers.LODCM, 'pcdsdevices.Motor': pcdsdevices.happi.containers.Motor, 'pcdsdevices.MovableStand': pcdsdevices.happi.containers.MovableStand, 'pcdsdevices.OffsetMirror': pcdsdevices.happi.containers.OffsetMirror, 'pcdsdevices.PIM': pcdsdevices.happi.containers.PIM, 'pcdsdevices.PulsePicker': pcdsdevices.happi.containers.PulsePicker, 'pcdsdevices.Slits': pcdsdevices.happi.containers.Slits, 'pcdsdevices.Stopper': pcdsdevices.happi.containers.Stopper, 'pcdsdevices.Trigger': pcdsdevices.happi.containers.Trigger, 'pcdsdevices.Vacuum': pcdsdevices.happi.containers.Vacuum}

So technically we are already doing that, right? Since you have the container class as the value of the dict you can get the module.classname ?
I agree, the entrypoint will allow for flexibility on this sense

FIX: Switch client to use registry instead of search at happi.containers. FIX: Address issues found during tests of the cli.

hhslepicka · 2020-03-21T00:56:22Z

I need to fix the tests and I am finding some more issues... it will take a while, folks! 😞

klauer · 2020-03-23T16:59:51Z

happi/containers.py

+                         for _, var in inspect.getmembers(obj, inspect.isclass)
+                         if issubclass(var, HappiItem)
+                         # Avoid happi internal classes due to imports
+                         and not var.__module__.startswith('happi.')})


Suggested change

and not var.__module__.startswith('happi.')})

and not var.__module__.startswith('happi.')} and var not in registry.values())

Also this check of "can we add it to the registry" is a repeated condition that could be made a function: i.e., it's duplicated on line 20 and in this dict comprehension

klauer · 2020-03-23T17:00:53Z

happi/containers.py

+
+_entries = entrypoints.get_group_all('happi.containers')
+
+registry = {}


Let's move the registry generation code inside a function, otherwise things like name, entry, obj, etc may leak such that from happi.containers import entry would work.

Will address that.

klauer · 2020-03-23T17:03:36Z

happi/containers.py

+        continue
+    if inspect.isclass(obj) and issubclass(obj, HappiItem):
+        registry[f"{name}.{obj.__name__}"] = obj
+    elif inspect.ismodule(obj):


To re-cap we have 2 options now:

Specify a class

Specify a (sub-)module to find classes in

Correct! I ended up adding support for both.

hhslepicka · 2020-04-16T00:52:20Z

Sorry for the delay... I reworked the registry after talking with Ken.
Also, at the client we no longer have the device_types. It now points to the registry always and for the cli.py we filter-out the deprecated Device. It is included at the registry to allow us to properly load old devices that we have.

Most likely the changes here will break the existing db.json at devices-config repo if the type field does not match a key at the registry.

Registry keys are now in the following format:

entry_point_name + class_full_qualified_name[1:]

This change will allow us to move/rename packages and still be able to find the entries with no changes to the database.

What is not done:

Aggressive search when a package is passed. E.g.: Look at all python files inside of a package directory.

codecov-io · 2020-04-16T01:01:38Z

Codecov Report

Merging #126 into master will decrease coverage by 2.98%.
The diff coverage is 54.28%.

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
- Coverage   75.57%   72.58%   -2.99%     
==========================================
  Files          15       15              
  Lines        1134     1120      -14     
==========================================
- Hits          857      813      -44     
- Misses        277      307      +30

Impacted Files	Coverage Δ
happi/item.py	`97.76% <ø> (+0.59%)`	⬆️
happi/cli.py	`45.45% <14.28%> (-0.31%)`	⬇️
happi/containers.py	`58.06% <58.06%> (-41.94%)`	⬇️
happi/client.py	`89.31% <100.00%> (+0.22%)`	⬆️
happi/device.py	`91.17% <0.00%> (-2.95%)`	⬇️
happi/backends/qs_db.py	`65.21% <0.00%> (-1.45%)`	⬇️
happi/loader.py	`93.10% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4978127...d71fabf. Read the comment docs.

klauer

I think this looks really good!

happi/client.py

klauer · 2020-04-16T17:16:48Z

happi/containers.py

+                               f"and class: {klass}")
+        if klass in self._reverse_registry:
+            dup_key = self._reverse_registry.get(klass)
+            raise RuntimeError(f"Duplicated entry found. Keys: {key} "


Nice and helpful 👍

klauer · 2020-04-16T17:20:58Z

(Nudging @ZLLentz)

ZLLentz

LGTM. Was confused about including pcdsdevices in the tests, but it makes sense.

hhslepicka · 2020-04-16T17:54:08Z

@ZLLentz I would gladly nuke that test out but it tests the Questionnaire backend and that directly depends on pcdsdevices to test the type of devices. Sorry about the confusion.

ZLLentz · 2020-04-16T18:06:37Z

@hhslepicka that backend should eventually migrate to a different repo... Surely there's a questionnaire-centered repo somewhere? But it's not an issue so there is no reason to prioritize it

Hugo Slepicka added 2 commits March 20, 2020 15:27

REF: Move LCLS containers out of Happi.

361b410

ENH: Allow containers via entrypoint happi.containers.

REF: Use more of inspect than brute-force.

5c39ffb

hhslepicka mentioned this pull request Mar 20, 2020

ENH: Use entry_points to vendor happi containers. pcdshub/pcdsdevices#402

Merged

klauer reviewed Mar 20, 2020

View reviewed changes

Hugo Slepicka added 2 commits March 20, 2020 17:40

FIX: Make OphydItem into the list and remove Device at cli.

80f93ca

FIX: Use package.classname to avoid name collision.

6865203

FIX: Switch client to use registry instead of search at happi.containers. FIX: Address issues found during tests of the cli.

klauer reviewed Mar 23, 2020

View reviewed changes

This was referenced Mar 23, 2020

Automated Container Creation #36

Open

Avoid requirement to register new device classes #85

Closed

Hugo Slepicka added 2 commits April 14, 2020 14:57

WIP: Fixing tests.

f467339

TST: Fix tests after changes on client and containers.

52202c5

BLD: Add missing dependency entrypoints.

200ffa5

klauer approved these changes Apr 16, 2020

View reviewed changes

REF: Remove unnecessary isclass check.

d71fabf

ZLLentz approved these changes Apr 16, 2020

View reviewed changes

klauer merged commit 7bb9fd3 into pcdshub:master Apr 16, 2020

hhslepicka mentioned this pull request Apr 16, 2020

Move Questionnaire Backend Out and Refactor Backend discovery #130

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Containers via entrypoints #126

ENH: Containers via entrypoints #126

hhslepicka commented Mar 20, 2020

klauer left a comment

klauer Mar 20, 2020

hhslepicka Mar 20, 2020

hhslepicka Mar 20, 2020

hhslepicka Mar 20, 2020

klauer Mar 20, 2020

hhslepicka Mar 21, 2020

klauer Mar 23, 2020

hhslepicka Mar 23, 2020

klauer Mar 23, 2020

hhslepicka Mar 23, 2020

klauer Mar 23, 2020

klauer Mar 23, 2020

hhslepicka Mar 23, 2020

hhslepicka commented Mar 21, 2020

klauer Mar 23, 2020

klauer Mar 23, 2020

klauer Mar 23, 2020

hhslepicka Mar 23, 2020

klauer Mar 23, 2020

hhslepicka Mar 23, 2020

hhslepicka commented Apr 16, 2020

codecov-io commented Apr 16, 2020 •

edited

Loading

klauer left a comment

klauer Apr 16, 2020

klauer commented Apr 16, 2020

ZLLentz left a comment

hhslepicka commented Apr 16, 2020

ZLLentz commented Apr 16, 2020


		_entries = entrypoints.get_group_all('happi.containers')

		registry = set()

	and not var.__module__.startswith('happi.')})
	and not var.__module__.startswith('happi.')} and var not in registry.values())


		_entries = entrypoints.get_group_all('happi.containers')

		registry = {}

ENH: Containers via entrypoints #126

ENH: Containers via entrypoints #126

Conversation

hhslepicka commented Mar 20, 2020

Description

Motivation and Context

How Has This Been Tested?

Where Has This Been Documented?

Screenshots (if appropriate):

klauer 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

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

hhslepicka commented Mar 21, 2020

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

hhslepicka commented Apr 16, 2020

codecov-io commented Apr 16, 2020 • edited Loading

Codecov Report

klauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klauer commented Apr 16, 2020

ZLLentz left a comment

Choose a reason for hiding this comment

hhslepicka commented Apr 16, 2020

ZLLentz commented Apr 16, 2020

codecov-io commented Apr 16, 2020 •

edited

Loading