pcdshub / hutch-python

Launcher and config reader for LCLS interactive IPython sessions
https://pcdshub.github.io/hutch-python/
Other
0 stars 18 forks source link

MNT: update lightpath use for object database loading #350

Closed tangkong closed 1 year ago

tangkong commented 2 years ago

Description

Motivation and Context

Closes #349

Should only be merged once other lightpath-related PR's are merged (eg pcdsdevices#1028)

Currently seems to cause a slowdown that I have not isolated. It may be to an increased number of devices being loaded, but testing has not been comprehensive

How Has This Been Tested?

interactively, with quite a bit of pathmunging

Where Has This Been Documented?

This PR

ZLLentz commented 1 year ago

Forgot about this PR! Should we wait to tag this repo until lightpath is tagged and available for the CI here?

ZLLentz commented 1 year ago

lightpath is already tagged actually, I'm going to close and re-open this to see what the CI says

tangkong commented 1 year ago

I totally forgot I did this too. I'll take another look

tangkong commented 1 year ago

fwiw these tests have been passing forever locally, but the experiments-loading bit is confounding travis. This seems to work loading lightpath and its objects if it does exist

ZLLentz commented 1 year ago

the experiments-loading bit is confounding travis

That's annoying but we can ignore it for this PR

ZLLentz commented 1 year ago

I think those test failures can be traced back to:

16:15:28.690 client          WARNING  MainThread Entry for tst_device_1 is malformed (The information relating to the container class has been modified to the point where the object can not be initialized, please load the corresponding document). Skipping.
16:15:28.706 client          WARNING  MainThread Entry for tst_device_2 is malformed (The information relating to the container class has been modified to the point where the object can not be initialized, please load the corresponding document). Skipping.

Which implies that the tst db file needs some additional reworking outside of this PR...

ZLLentz commented 1 year ago

Or maybe it means that the tests need happi v2 to pass

tangkong commented 1 year ago

No I'm dumb, you're right. The container needs to not rely on lightpath as well, while still working if lightpath exists.

I had switched to LCLSLightpathItem to be lightpath-compatible, but that may not exist

ZLLentz commented 1 year ago

is lightpath not a dependency of hutch-python? I guess it's an optional? We can just force it to be included in the test suite run

tangkong commented 1 year ago

I'll give that a shot then

tangkong commented 1 year ago

Ah ok at this point we're waiting for pcdsdevices tag to pull the new LCLSLightpathItem container.

ZLLentz commented 1 year ago

Gotcha, that should be tagged today but I doubt it will make it through the Friday conda-forge pipeline quickly

tangkong commented 1 year ago

With https://github.com/pcdshub/pcdsdevices/pull/1065 I'm reverting the containers to LCLSItem, but hopefully leaving the device_class as a SimpleNamespace so that the branches are accessible after being instantiated.

Let's see how fast the tag shows up for pypi

tangkong commented 1 year ago

This has turned into a test-suite touchup PR. There's a lightpath bugfix that lets LightController properly handle user supplied configs, but until that is in and tagged some of these tests will be skipped.

This might warrant a quick test run before we merge this, but I've tested loading using a conf.yml that points to an updated happi database

ZLLentz commented 1 year ago

I think this is good to go, right?

tangkong commented 1 year ago

Thanks for the merge 👍