Deep update of specs in Lancium adapter

[giffels]

Currently, the spec dictionary in the Lancium adapter is not "deeply" updated. For example:

TARDIS is setting up the Drone specific environment in the spec dictionary:

spec = {"environment": [{"value": "8", "variable": "TardisDroneCores"},
                                        {"value": "20", "variable": "TardisDroneMemory"},
                                        {"value": "20", "variable": "TardisDroneDisk"},
                                        {"value": "testsite-089123", "variable": "TardisDroneUuid"}]
            }

However, if the user decides to set-up a specific environment in the configuration, like:

spec = {"environment": {"value": "T1_DE_KIT/KIT-Lancium",
                                        "variable": "SITECONFIG_PATH"},
            }

this is currently going to overwrite the environment specfied by TARDIS.

This pull request uses a deep update function to ensure, that the environment is updated instead of being overwritten.

[codecov-commenter]

Codecov Report

Base: 98.88% // Head: 98.88% // No change to project coverage 👍

Coverage data is based on head (a6566c4) compared to base (3f88e76).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #276   +/-   ##
=======================================
  Coverage   98.88%   98.88%           
=======================================
  Files          56       56           
  Lines        2325     2325           
=======================================
  Hits         2299     2299           
  Misses         26       26           

Impacted Files	Coverage Δ
tardis/adapters/sites/lancium.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[maxfischer2781]

I've added some comments on the deep_update implementations. I'm very certain on the typing considerations but feel free to ignore the algorithm suggestions/changes.

[maxfischer2781]

I think technically the original must be MutableMapping since we need to change items in the copy. Consider to even restrict it to Dict since that's what is substituted at some points and you also special-case List (instead of Sequence).
For mapping_update it's fine.

In the interest of generality, you might want to support List as well – that would allow a recursive deep_update into lists as well. Only if you want the extra work, though...

[giffels]

I have changed the original to MutableMapping. I thought to support List's as well, however I concluded that there is currently no need for it , so I decided to implement only the minimal solution working for us.

[giffels]

I've added some comments on the deep_update implementations. I'm very certain on the typing considerations but feel free to ignore the algorithm suggestions/changes.

Thank you @maxfischer2781 for the in depth review. I will change things or reply to the inline comments otherwise.

[maxfischer2781]

Feel free to update the master.

[maxfischer2781]

maxfischer2781 force-approved the fix-lancium-adapter branch

[rfvc]

Is extend really what I expect deep_update to do? Running

original_mapping = {"foo": [1, 2, 3], "bar": "baz"}
mapping_update = {"foo": [4, 5, 6]}
updated_mapping = deep_update(original_mapping, mapping_update)

I naively would expect updated_mapping to be {"foo": [4, 5, 6], "bar": "baz"} and not {"foo": [1, 2, 3, 4, 5, 6], "bar": "baz"}. This behavior I could achieve by explicitly running

original_mapping = {"foo": [1, 2, 3], "bar": "baz"}
mapping_update = {"foo": list(set(original_mapping["foo"] + [4, 5, 6]))}
updated_mapping = deep_update(original_mapping, mapping_update)

Maybe this can be accomplished by explicitly setting some extend_lists-flag (possibly with True as default) in the call of deep_update?

[giffels]

I think to implement deep_update was not a very wise idea and I am not happy with it as well. It seems to be prone to errors and to misunderstandings. Since, we know exactly what needs to be appended or overwritten in the Lancium SiteAdapter, I will implement it "manually" in the adapter itself following KISS.

[maxfischer2781]

Have to agree that using an implementation specific to Lancium instead of trying to formulate a generic case seems more appropriate, given how many edge cases we run into otherwise. 😕

[maxfischer2781]

That's... simpler. 😁

[rfvc]

I agree with @maxfischer2781, that looks a lot simpler 😅 Thanks @giffels!