-
Notifications
You must be signed in to change notification settings - Fork 0
feat: enable config of node-specific fields in vlm response (Issue #27) #39
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
base: main
Are you sure you want to change the base?
feat: enable config of node-specific fields in vlm response (Issue #27) #39
Conversation
…ponse' classes that have statically-created fields
|
@jennifer-bowser could you try pulling in changes from main to see if it fixes CI? |
…ds-in-vlm-response
…butes via env vars
…nd into 'build_vlm_response_from_caf_data'
| model_config = ConfigDict(populate_by_name=True) | ||
|
|
||
|
|
||
| class MetaSettings(BaseSettings): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this is here and not in the config module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I couldn't figure out how to use the config module to pull the beaconId from environment variables in a way that would throw an error if it's not set while also preventing this field from ever being overriden, while also making sure typecheckers know that you're not supposed to pass in a value for beaconId when instantiating the class. I also wanted to keep the metadata associated with the description field for this.
I've tried a couple different things, but this is the best I was able to figure out. I don't really love it though, I'd very much be open to suggestions if you have any!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... I am not sure I understand the tradeoffs, but it probably doesn't matter. I think we are likely in "merge and deal with it" territory
The schema classes defined in
src/anyvlm/schemas/vlm.pyhave 4 variables that we will be hard-coding to GREGoR-specific values for our MVP release. These can be divided into 2 groups:Metadata about the source the CAF data is coming from:
HandoverType.idHandoverType.labelBeaconHandover.urlMetadata about the AnyVLM node:
Meta.beaconIdSince our only data source for MVP will be GREGoR, hard-coding values for the items under Point Number 1 works for the time being. However, we'll ultimately need to pull these three values dynamically (since we'll be adding support for nodes to contain data from multiple sources post MVP - see Issue #37). I've therefore set these via environment variables for now in
build_vlm_response_from_caf_data()for expediency, but have left #TODOs referencing the multi-cohort epic in the places where I'm using them.The item under Point Number 2 (i.e.
Meta.beaconId) should always be set once per node and should not change (even post-MVP). This one makes sense to leave an environment variable long-term, so I handled this differently by pulling the env var in at the class level and using Pydantic validation to ensure it's set..env.examplefile for the new env var names and the GREGoR default values we'll use for our MVP release. You'll need to add these to your own.envfile in order for tests to pass.