FEAT: Standardizing AIRTInitializer#1578
Conversation
behnam-o
left a comment
There was a problem hiding this comment.
some little changes/suggestions
pyrit/setup/initializers/airt.py
Outdated
| ValueError: If mandatory global memory labels are missing. | ||
| """ | ||
| config_path = Path.home() / ".pyrit" / ".pyrit_conf" | ||
| with open(config_path) as f: |
There was a problem hiding this comment.
I'm not 100% sure how configuration loader and initializers tie into each other, but it sounds like we have configuration_loader to avoid parsing yml directly? in that case can we use that to resolve these lables?
( skimming throught config loader, it looks like config loader references an initializer, which made me think: maybe moving this whole validation function to the config loader makes more sense? but the other validation on require_env_vars is happening here, so maybe this is a good place for label validation too ... hence why I don't have a strong opinion)
There was a problem hiding this comment.
This is a great design question that I think is out of scope for this PR, but I want to discuss it. It's a bad pattern to have the config file feed configuration_loader which then calls an initializer that just looks at the config file again, but there's three issues we'd need to address first:
- What are initializers allowed to do? Right now, initializers have no bounds. They are basically just scripts. This isn't necessarily a bad thing since the flexibility is useful, but we have implicit contracts for ordering of initializers that aren't strictly enforced. If we decide that initializers can't look backwards in the call chain, I think that decision needs to be formalized.
- Should
initialize_asyncpropagate config values? I think the answer should be yes, but I can see design reasons for it being no. Inconfiguration_loader.pywe haveinitialize_from_config_asyncwhich callsConfigurationLoader.initialize_pyrit_async, which itself only propagates config file contents implicitly if they're arguments passed to an initializer. Whether or not we want config values visible downstream is an open question. - Much smaller, but for long-term re-usability, are
.pyrit_conffiles composable? Can we have multiple of them for different profiles? Unlike environment files we actually probably do want to make it easier to swap / modify / compose them frequently and our current design doesn't let that happen.
I think for this PR it's fine to reference the config explicitly since the existing usage pattern is "an initializer is a script that does whatever you need it to." But I don't think that's extensible long-term. Feel free to disagree though, I want to hear your thoughts.
Description
A very small PR to standardize the AIRT initialization flow.
AZURE_SQL_DB_CONNECTION_STRINGandAZURE_STORAGE_ACCOUNT_DB_DATA_CONTAINER_URL.GLOBAL_MEMORY_LABELSis used to store username and current operation name. For backwards compatibility, these areoperatorandoperationin.pyrit_conf, andusernameandop_namein the memory labels respectively.GLOBAL_MEMORY_LABELS. Users seeking to change the operation name can do so by changing the value in.pyrit_confand re-running the initializer.operationandoperatorin the config to prevent regression.Tests and Documentation
tests.unit.setup.test_airt_initializer.