instrumentkit / InstrumentKit

Python package for interacting with laboratory equipment over various buses.
250 stars 71 forks source link

Update ruamel.yaml to >=0.18 #407

Closed trappitsch closed 9 months ago

trappitsch commented 9 months ago

Part of #406

Currently I'm using the unsafe loader, which would be okay since the docs specify that this is not safe for files you don't trust.

However, we could try and register all ik classes with the yaml loader, see here. Scratching my head at the moment though why I can't even get the tests to run when I register instruments.Instrument with

from instruments.config import yaml

@yaml.register_class
class Instrument
    ...

Any ideas?

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (e7797c0) 99.03% compared to head (be0261f) 99.12%. Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #407 +/- ## ========================================== + Coverage 99.03% 99.12% +0.08% ========================================== Files 88 88 Lines 8993 8989 -4 ========================================== + Hits 8906 8910 +4 + Misses 87 79 -8 ``` | [Flag](https://app.codecov.io/gh/instrumentkit/InstrumentKit/pull/407/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=instrumentkit) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/instrumentkit/InstrumentKit/pull/407/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=instrumentkit) | `99.12% <100.00%> (+0.08%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=instrumentkit#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

scasagrande commented 9 months ago

Any ideas?

Nope, I'm not familiar with the process by which we would need to register classes for yaml.

trappitsch commented 9 months ago

Well, I'm gonna play with this more. The current implementation is as unsafe/safe (depending on your standpoint) as before. Being able to register classes would definitely help this. Might have to play with some actual hardware to figure out why the test don't pass in my first tests with this.

trappitsch commented 9 months ago

So, looking at this in more details, it seems like we are not loading classes (which could be excepted from the safe loader), but we are loading module.name with !!python/name tags. The only way I can see to get this to work without restructuring a lot is to use typ="unsafe" in the loader.

We have the following comment in the docstring of config.load_instruments:

    .. warning::
        The configuration file must be trusted, as the class name references
        allow for executing arbitrary code. Do not load instruments from
        configuration files sent over network connections.

        Note that keys in sections excluded by the ``conf_path`` argument are
        still processed, such that any side effects that may occur due to
        such processing will occur independently of the value of ``conf_path``.

The previous version with the older ruamel.yaml used unsafe loading by default.

trappitsch commented 9 months ago

Added some tests to fully cover the config.load_instruments. However, submitting to codecov seems to be failing... the latest action is v4 (here) and we are using v2. Could this be an issue?

scasagrande commented 9 months ago

As far as I can tell, v2 is still supported, I only see reports that v1 doesn't work anymore as of 2022.

trappitsch commented 9 months ago

let's see if it's just flaky...

trappitsch commented 9 months ago

Here we go :)