Importing containers can result in conflicts during entry point containercollection

[tangkong]

Description

Following the discussions below, this is now a documentation issue. We should describe to users how to import containers from other packages without encountering the entry point conflict.

In short, if you wanted to use package.happi.containers.MyContainer in another package, you should do something similar to:

import package.happi.containers

class NewContainer(package.happi.containers.MyContainer):
    pass

Context

The conflict shows itself as:

(pcds-5.4.1)roberttk@psbuild-rhel7-01:~/devrepos/pcdsdevices(enh_lp_mixin +)$ happi container-registry
File "/cds/home/r/roberttk/devrepos/happi/happi/containers.py", line 208, in load
    self._safe_add(entry_name, var)
  File "/cds/home/r/roberttk/devrepos/happi/happi/containers.py", line 173, in _safe_add
    raise RuntimeError(f"Duplicated entry found. Keys: {key} "
RuntimeError: Duplicated entry found. Keys: pcdsdevices.happi.containers.LightpathItem and lightpath.happi.containers.LightpathItem point to same class: <class 'lightpath.happi.containers.LightpathItem'>

When you import from lightpath.happi.containers import LightpathItem in pcdsdevices.happi.containers

It seems the container gets collected once in lightpath and again in pcdsdevices under different keys, since they're being collected from different entrypoints. I can add an issue for it

Originally posted by @tangkong in pcdshub/lightpath#133 (comment)

[klauer]

I gave this a bit more thought there are a couple ways you could resolve the initial problem without changes to happi:

1. Adjust the import style in lightpath
   a. happi uses inspect.getmembers() on the module
   b. So, if the conflicting container type is not found in - effectively globals() - then it should not be found by that mechanism
   c. Then: from lightpath.happi import lightpath_containers and LCLSLightPathItem(lightpath_containers.LightpathItem)
2. Much less preferably: we could be more specific in pcdsdevices and only export happi container classes by name (you have the choice of class or module in the entrypoint)

A broader question would be: "is it OK to have aliases to containers?"
I think I'd like to continue to say that the answer is "no", because it could cause confusion down the line, make finding items of the same type more difficult, and so on. This is definitely open for discussion.

[klauer]

Thoughts, @tangkong?

[tangkong]

I totally thought I left a comment here whoops

I think 1 is acceptable, and is probably the simplest solution. It will be an additional thing to communicate/document, since it's not obvious that you can't just import the class you need to inherit from it. (Every package in the future that wants to expose inheritable containers will have to do this)

Could we change how we handle collisions? The first thought is to just ignore the duplicate entry, but I'm not sure if the order packages are collected in is consistent, which might make the key indeterminate. We could set up some logic (take the name that's first in alphabetical order?) to make it consistent I guess.

I agree that aliases to containers (several keys to the same container) will lead to confusion

I think on balance we can probably go with 1 in the short term, and is probably what we should do until we find a more elegant solution (if there is one)

[klauer]

Could we change how we handle collisions? The first thought is to just ignore the duplicate entry, but I'm not sure if the order packages are collected in is consistent, which might make the key indeterminate. We could set up some logic (take the name that's first in alphabetical order?) to make it consistent I guess.

My take on it - which still could be potentially flawed:

The order of loading is likely inconsistent and not necessarily what we would hope: so we can't rely on it. Because we can't rely on it, we can't know if "pcdsdevices.LightPathItem" is the canonical one or "lightpath.LightpathItem" is the canonical one.

You could maybe argue that maybe we should check the module it came from and use that one - but what if we see the pcdsdevices entrypoint before we see lightpath: that gets registered and then we overwrite it with the lightpath one? It's rather confusing.

I think my suggestion (2) above is a reasonable way around this scenario, though I like it less because it makes exporting containers a bit more labor-intensive.

So then the guidance for users who import other containers would be (summarizing from the above):

1. Use module.Container to avoid collisions
2. Or specify only your exported containers in setup.py by class

[tangkong]

Agreed on all points. I'm coming to terms with (1) being simple enough as long as people read the documentation we write. 😬