Skip to content

FEAT: Standardizing AIRTInitializer#1578

Open
ValbuenaVC wants to merge 11 commits intomicrosoft:mainfrom
ValbuenaVC:airt_initializer
Open

FEAT: Standardizing AIRTInitializer#1578
ValbuenaVC wants to merge 11 commits intomicrosoft:mainfrom
ValbuenaVC:airt_initializer

Conversation

@ValbuenaVC
Copy link
Copy Markdown
Contributor

@ValbuenaVC ValbuenaVC commented Apr 7, 2026

Description

A very small PR to standardize the AIRT initialization flow.

  • Default storage location is set to an Azure SQL database using AZURE_SQL_DB_CONNECTION_STRING and AZURE_STORAGE_ACCOUNT_DB_DATA_CONTAINER_URL.
  • GLOBAL_MEMORY_LABELS is used to store username and current operation name. For backwards compatibility, these are operator and operation in .pyrit_conf, and username and op_name in the memory labels respectively.
  • Everything propagates through GLOBAL_MEMORY_LABELS. Users seeking to change the operation name can do so by changing the value in .pyrit_conf and re-running the initializer.
  • The initializer requires values for operation and operator in the config to prevent regression.

Tests and Documentation

  • Added basic tests to tests.unit.setup.test_airt_initializer.

Victor Valbuena added 2 commits April 8, 2026 23:19
@ValbuenaVC ValbuenaVC marked this pull request as ready for review April 8, 2026 23:23
@ValbuenaVC ValbuenaVC changed the title [DRAFT] FEAT: Standardizing AIRTInitializer FEAT: Standardizing AIRTInitializer Apr 8, 2026
Victor Valbuena added 2 commits April 8, 2026 23:34
Copy link
Copy Markdown
Contributor

@behnam-o behnam-o left a comment

Choose a reason for hiding this comment

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

some little changes/suggestions

ValueError: If mandatory global memory labels are missing.
"""
config_path = Path.home() / ".pyrit" / ".pyrit_conf"
with open(config_path) as f:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor Author

@ValbuenaVC ValbuenaVC Apr 9, 2026

Choose a reason for hiding this comment

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

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_async propagate config values? I think the answer should be yes, but I can see design reasons for it being no. In configuration_loader.py we have initialize_from_config_async which calls ConfigurationLoader.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_conf files 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.

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.

4 participants