Skip to content

[WIP] Extended YAML support#162

Closed
cgranade wants to merge 12 commits into
masterfrom
feature-extended-yaml
Closed

[WIP] Extended YAML support#162
cgranade wants to merge 12 commits into
masterfrom
feature-extended-yaml

Conversation

@cgranade

Copy link
Copy Markdown
Contributor

This PR (still work in progress) adds to existing YAML config-file support by updating it for Py3 and by allowing config files to specify attributes to be set on newly-created instruments. This is useful, for instance, in the case of ThorLabs APT motor controller devices, where the model of each attached motor must be manually specified. Since this is a fairly significant set of changes, I wanted to open the PR in advance of writing tests and better documentation so as to get feedback on the design.

In particular, this PR adds the following functionality:

  • A new special test:// instrument URI schema to make it easier to test config support in lieu of actual devices.
  • Support for instantiating unitful numbers from within YAML files, with a new !Q tag.
  • Support for new attrs config sections of the following form:
instrument_id:
    class: !!python/name:instruments.ClassName
    uri: schema://inst
    attrs:
        foo: bar
        baz[0].blah: !Q 1 tesla
  • Slight refactoring to the ThorLabs APT classes to allow for better use with config files (backwards-compatible, guarded with a DeprecationWarning).

I intend on adding the following to the PR:

  • Removing all pylint warnings introduced.
  • Writing tests for the new attr minilanguage, YAML tags, test:// URI, and attrs config sections.
  • Adding references to new functionality to the user guide.

@coveralls

coveralls commented Apr 12, 2017

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.2%) to 84.419% when pulling eaec515 on feature-extended-yaml into a0d9088 on master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.2%) to 84.885% when pulling 10915e6 on feature-extended-yaml into a0d9088 on master.

2 similar comments
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.2%) to 84.885% when pulling 10915e6 on feature-extended-yaml into a0d9088 on master.

@coveralls

coveralls commented Apr 12, 2017

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.2%) to 84.885% when pulling 10915e6 on feature-extended-yaml into a0d9088 on master.

@scasagrande scasagrande left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fantastic starting point! LOVE IT

A few minor things I saw while riding the bus are noted.

Should we take this opportunity to upgrade from pyyaml to something still being developed? I know you sent me the link to another project, but I don't have it on hand at the moment...

Also, could you switch the merge target to develop, thanks :)

Comment thread instruments/config.py

inst_dict = {}
for name, value in conf_dict.iteritems():
for name, value in conf_dict.items():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

Comment thread instruments/config.py
the form
``{'ddg': instruments.srs.SRSDG645.open_from_uri('gpib+usb://COM7/15')}``.

Optionally, each the value of each instrument key can also contain a dictionary

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awkward sentence

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll improve that, then.

Comment thread instruments/config.py
rot_stage:
class: !!python/name:instruments.thorabsapt.APTMotorController
uri: serial:///dev/ttyUSB0?baud=115200
attrs:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I LOVE IT


# pylint: disable=protected-access,missing-docstring

@skipIf(yaml is None, "PyYAML is not installed.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be skipping tests if pyyaml is missing. I can see the situation where it fails to install in CI (for whatever reason) and simultaneously there is a bug that these tests would reveal. Build would be green, even though some tests technically fail.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, I'll make sure YAML (whichever once we go with) is appropriately a dependency for the test environment, then. I think now that the conda command depends on rumel.yaml, it's not as heavy of a dependency as it used to be.

@cgranade

cgranade commented Apr 13, 2017

Copy link
Copy Markdown
Contributor Author

Thanks, I'm glad you like! Thanks as well for the feedback, I'll go on and fix that stuff up, then. (Sorry about the wrong target for the PR, I always forget to change from the GitHub default... have they introduced a setting to control that yet? Will close PR and retarget.)

@cgranade cgranade closed this Apr 13, 2017
@cgranade cgranade mentioned this pull request Apr 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants