pcdshub / btms-ui

Laser Hall Beam Transport Motion System User Interface
Other
0 stars 4 forks source link

PR to support a more flexible camera naming. #16

Closed slactjohnson closed 1 month ago

slactjohnson commented 2 months ago

Description

Modify the btms to use a new property of the btms_config.SourcePosition class, introduced in this PR on pcdsdevices: https://github.com/pcdshub/pcdsdevices/pull/1265

Motivation and Context

closes #15

How Has This Been Tested?

Tested interactively in the laser hall.

Where Has This Been Documented?

This PR.

tangkong commented 1 month ago

The version handling here broke for some reason, which is why the CI is failing

tangkong commented 1 month ago

This might be related to the comment zach made in #9 about _version.py being auto-generated and not being needed to be tracked in the repository.

Our cookiecutter is setup to have a version.py file (no underscore) instead of _version.py (with underscore). Can you try changing the name of that, and modifying __init__.py to match the cookiecutter version?

tangkong commented 1 month ago

Playing around with this locally, I think this breaks because the tag format is not recognized by setuptools_scm. The prepended R breaks the tag parsing.

> /cds/group/pcds/pyps/conda/py39/envs/pcds-5.8.4/lib/python3.9/site-packages/setuptools_scm/version.py(210)meta()
    208     node_date: date | None = None,
    209 ) -> ScmVersion:
--> 210     parsed_version = _parse_tag(tag, preformatted, config)
    211     log.info("version %s -> %s", tag, parsed_version)
    212     assert parsed_version is not None, "Can't parse version %s" % tag

ipdb> tag
'R1.0.0'

ipdb> _parse_tag('1.0.0', preformatted, config)
<Version('1.0.0')>

Are we committed to RX.X.X as the tag format? Or could we use 1.0.0?

slactjohnson commented 1 month ago

If prefixing with 'R' is problematic, then I'm happy to change it. I just used the 'Rx.y.z' format because that's historically how we've tagged our IOC releases. We can still name the release directory "R.x.y.z" if we want.

tangkong commented 1 month ago

Unfortunately the 'Rx.y.z' format isn't seen anywhere else from what I've seen. Changing the release tag format is the best path forward for this python repo at least.

For the purposes of this PR, we can ignore the CI and bypass the checks

slactjohnson commented 1 month ago

Thanks! I'll make this ready for review then and make a new tag without the 'R' after the merge.

tangkong commented 1 month ago

Requesting a review from @ZLLentz to double check my opinion on the tag / versioning issue

ZLLentz commented 1 month ago

RE: tag/versioning issue, python has a strict, explicit spec for version identifiers. Details here: https://packaging.python.org/en/latest/specifications/version-specifiers

Summary:

ZLLentz commented 1 month ago

The core change here is :+1: just the packaging stuff

slactjohnson commented 1 month ago

Thanks. It seems like I do not possess the power to merge without approving review and passing tests

tangkong commented 1 month ago

Builds after the new, compliant tag name look good. Now only failing on a pypi / conda deployment we haven't setup 🎉