Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data objects #12

Closed
wants to merge 1 commit into from
Closed

Data objects #12

wants to merge 1 commit into from

Conversation

cjw296
Copy link
Member

@cjw296 cjw296 commented Jul 15, 2022

This will allow .data to be lazy loaded, and eventually record its origin.

@cjw296 cjw296 force-pushed the data_objects branch 2 times, most recently from 81915fd to ad1ffec Compare July 20, 2022 18:10
tests/test_functional.py Outdated Show resolved Hide resolved
@@ -87,6 +90,81 @@ def test_overlay(self, dir):
compare(config.user, expected=2)
compare(config.file, expected=3)

def test_lazy_load(self, dir):
Copy link
Member Author

Choose a reason for hiding this comment

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

@RonnyPfannschmidt - okay, try this now. I think formalising the Proxy object gives a really nice clean outcome, but interested to hear what you think. If you want, I think you should be fine to stick the proxy object on the config root too :-)

Choose a reason for hiding this comment

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

from my pov this is basically a drunk identity copied contextvar,

hence the desire to access the config root and path of the dynamic lookup

the tricky point about the client landing in data however is a valid concern, however i would currently consider it less of a problem than having the loader proxy object be integrated differently

how about the concept of a Sidechannel value

it would be a Contant Data Value that one could update to a object for within a Config, but uppon copying a config, it would
"loose" the Value

then it would no longer be a contextvar, but rather a symblic reference one could map to other objects

Copy link
Member Author

Choose a reason for hiding this comment

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

"from my pov this is basically a drunk identity copied contextvar," comes across quite badly, I'm trying to help here. I don't need this functionality myself, and I'm struggling to understand the resistance to the suggestions I'm making.

I haven't used ContextVar so maybe this is a good opportunity for me to learn.

I can't get my head around your Con(s?)tant Data Value idea, but it seems to be related to your fixation with storing vault_loader on the Config object. That's not something I really like, but I'd like to understand why it's important to you so that we can find something that keeps both of us happy :-)

Choose a reason for hiding this comment

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

i would be fine with not storing the vault loader on the config,
a key consideration for me is how to make it available to the loaded value (which ideally doesn't need a extra magical object

having a proxy object thats tricky to manage and weaseling it into a dynamic loader is something i would like to avoid

im under the impression we have a very different understanding of how to pass the context in (my preference would be to just pass the root config object)

your preference seems to be to pass in something like a context variable, however the proposed variant looks to me like it will carry context over into new copies of the configuration (which seems to be a bug NO)

the idea of the "standin values" would be to have a way to have a sidechannel for the configruation

so that if one had a config like

symbol = Symbol("vault-loader")
config = Config({"password": VaultLoader(loader = symbol, key="password"))
symbol.set_for(config, VaultClient.for_config(config))
assert config.password = "abc123#supersecure"

Copy link
Member Author

@cjw296 cjw296 Jul 22, 2022

Choose a reason for hiding this comment

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

@RonnyPfannschmidt - I hope it's not intentional, but your tone is still coming across quite negatively... "magical", "weaseling" and "bug NO" are not things that make this fun for me to work on.

I just read up on ContextVar, and while the API may appear similar, for me, the Proxy here is more like a Twisted Deferred. It's a proxy for an object because we don't have that object when we need to pass around a reference to it. They only need to be referenced in the config parsing code, so not sure how that equates to being tricky to manage. The dynamic loader needs a reference to the vault client before the vault client can be instanted, so having a proxy reference to it which case be filled in later felt quite natural to me.

Unfortunately, the notion of when to carry over a Proxy into a new config is a tricky one. I'm interested in where that would cause a problem?

Choose a reason for hiding this comment

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

Most likely the same way as in symbol, which is just a name for proxy

Copy link
Member Author

@cjw296 cjw296 Jul 25, 2022

Choose a reason for hiding this comment

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

get_context in your example appears to only be needed if you require path traversal, which I think we both agreed was a bad thing? (The path can be out of sync with where the node is in the config).

I'm trying to figure out a way the Proxy can have a value per config without needing a direct reference to the config, since I'm still not sure that can be plumbed through: configurator has always tried to not get involved with the underlying config data (although I think #9 may have to change that...), so .data never has access to a node, it's always the ConfigNode that refers down to the .data, and the nodes are basically ephemeral.

Choose a reason for hiding this comment

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

it cannot be done with get - get_context is intended to pass the config trough to each constructed node, so it can be used in the lazy resolving

Copy link
Member Author

Choose a reason for hiding this comment

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

What, specifically, cannot be done with get?

Choose a reason for hiding this comment

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

Obtaining the config object that was in use for the lookup

@cjw296
Copy link
Member Author

cjw296 commented Aug 31, 2022

@RonnyPfannschmidt - I'm afraid I lost time to work on this, but wanted to check in: are the issues you raised in July still relevant or have you found a solution using a package other than configurator?

@RonnyPfannschmidt
Copy link

Currently I am on parental leave,
No alternative has been found but the migration is not "pressing" in the sense that the non configurator version is not broken

@cjw296
Copy link
Member Author

cjw296 commented Aug 31, 2022

Cool, let me know when this next comes to the stop of your stack and I'll let you know how I'm doing :-)
(Oh, and congratulations! 🎉 )

@cjw296
Copy link
Member Author

cjw296 commented Jan 7, 2023

I'm going to close this one off, having come back to it after a good break, and with some other use cases that have come up, I'm coming round to the idea that directly accessing .data may not be something Configurator can still support while having the richer functionality various folks are after.

@cjw296 cjw296 closed this Jan 7, 2023
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.

2 participants