MatterMiners / cobald

Cobald is an Opportunistic Balancing Deamon
https://cobald.readthedocs.io
MIT License
11 stars 12 forks source link

YAML tags do not work with nested mapping or sequence nodes #99

Closed giffels closed 2 years ago

giffels commented 2 years ago

Currently the COBalD YAML tags do not work with nested mapping or sequence nodes like:

!Foo
Bar:
  - Bla: Blubb

which constructs a python object Foo(Bar=[]) instead of Foo(Bar=[{"Bla": "Blubb"}]).

The later can be archieved by adding the deep=True argument to construct_mapping in https://github.com/MatterMiners/cobald/blob/094653385f942fa9f912c2602b8d1ff4ba7419e9/src/cobald/daemon/config/yaml.py#L50 and construc_sequence in https://github.com/MatterMiners/cobald/blob/094653385f942fa9f912c2602b8d1ff4ba7419e9/src/cobald/daemon/config/yaml.py#L55.

Could this be added to COBalD YAML tags, please?

maxfischer2781 commented 2 years ago

I've tried reproducing this (see #100) and failed gloriously. It just works. :/

However, it's unclear to me how deep=True actually works – its only effect seems to be whether constructed generators are evaluated within the construction stack or afterwards. So as far as I can tell it means whether the translation happens in a deep call stack or not.
But there's nothing in the docs on deep so I might just be reading the code wrong.

For reference, the code to interpret !!python tags all use deep=True. I assume that deep=True is what we should use to avoid out-of-order evaluation, I'm just not sure how a config would actually contain this or how to test it.

maxfischer2781 commented 2 years ago

Hm, I've found some tickets/PRs that used deep=True in combination with YAML anchors. Are there any in your original case?

giffels commented 2 years ago

There are no YAML anchors involved in my original case, which is parsing the following construct.

tardis:
  Services:
    restapi:
      !TardisRestApi
      host: 127.0.0.1
      port: 1234
      secret_key:  .....
      algorithm: HS256
      users:
        - user_name: tardis
          hashed_password: .....
          scopes:
            - user:read

However, I found the following explaination.

"The deep parameter indirectly controls whether objects that are potentially generators are recursively being built or appended to the list self.state_generators to be resolved later on.

Constructing a YAML document then boils down to constructing the top-level objects and looping over the potentially recursive objects in self.state_generators until no generators are left (a process that might take more than one pass)."

maxfischer2781 commented 2 years ago

So I think finally grokked this – completely misunderstood how the config is applied.

Since COBalD itself post-processes its entire config section, it doesn't matter when individual parts are created relative to one another. While some parts are constructed during config reading, they don't actually do anything with their arguments until the core application starts. That's similar to what I used for the unittests, where I loaded the entire config and then checked what's there.
That's not what TARDIS is doing: Since objects already peek into their configuration during loading, they expect the content to be there. So when you look at things they aren't there yet, but when my tests look at things later on they are constructed either way.


I'll try and adjust the tests to repro the timing issue. My plan would then be to provide an eager or lazy flag for the YAML tag plugins; the default would be to eagerly (deep=True) evaluate the part of the config.