Gemini Changes - #249
Gemini Changes#249
Conversation
… ids) to short or long
…ding-strings-to-model
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #249 +/- ##
==========================================
- Coverage 96.88% 96.47% -0.41%
==========================================
Files 88 88
Lines 4944 5164 +220
Branches 439 488 +49
==========================================
+ Hits 4790 4982 +192
- Misses 103 119 +16
- Partials 51 63 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I've beefed up the test coverage. Fair warning; the last of the auth ones and all of the user ones were AI generated via co-pilot. I think it's in a much better place now, I'll ping it back over to you. |
|
oh one thing I haven't actually done yet is test all the export stuff (post-merge) via Postman. I'll do that first thing tomorrow. |
I've checked this manually, and all works as expected. Just didn't want to rely solely on the automated test is all. |
… channel processing from scalar and string CSV handling.
|
ebc0b76: noticed a logging issue in which string and scaler values were falling through the if statement, this meant 1000's of duplicated logging lines. This just makes things more explicit. |
patrick-austin
left a comment
There was a problem hiding this comment.
Had a look over it again and could only find one (very) minor formatting change I'd definitely make (test_get_crosshair_intensity.py).
The rest are things that I noticed and wanted to check you're happy with, but in principle I don't think necessarily need to change if you are. As far as I could see the codecov report is OK given that we won't have the Gemini suite contributing to the coverage (I didn't consider on my last review until you pointed it out). I've left some thoughts about that as well but think it would be fine to let it lie (either for this PR and pick up as a separate issue, or for the foreseeable future).
One final thing - I was asking Kevin what bits of the API code would be covered by the "data integrity" testing he's been doing and if he knew what version/commit the Gemini instance is running, since I wasn't sure if any changes made to this branch would affect things for him. I think the answer was essentially "no" since he wasn't using export, and I guess at some point we'd have to deploy the latest version of the API to the Gemini instance anyway so it would get tested in pre-prod whenever that happens.
Co-authored-by: patrick-austin <61705287+patrick-austin@users.noreply.github.com>
|
Thanks, have made the appropriate changes. With regards to Kevin's testing; As you mentioned, he is only testing the data, not the version/functionality of OG. |
patrick-austin
left a comment
There was a problem hiding this comment.
Changes look good - just had one comment about the custom validator for the user office config. I think it's fine as is but was wondering if an optional "sub-model" would let us get the same functionality without having to write/test a custom validator? If there's a reason I'm missing why that wouldn't work, or you've already considered it and prefer it this way then I'm happy to approve, but thought it was worth raising for consideration at least.
| @model_validator(mode="after") | ||
| def check_user_office_settings(self) -> "AuthConfig": | ||
| """ | ||
| When the User Office integration is enabled, both the API key and the users | ||
| service URL are needed to contact it | ||
| """ | ||
| if self.user_office_integration: | ||
| missing = [ | ||
| name | ||
| for name in ("user_office_api_key", "user_office_users_service_url") | ||
| if not getattr(self, name) | ||
| ] | ||
| if missing: | ||
| raise ValueError( | ||
| f"{', '.join(missing)} must be set when " | ||
| "'user_office_integration' is enabled", | ||
| ) | ||
|
|
||
| return self |
There was a problem hiding this comment.
An alternative way of doing this could be to define an optional UserOfficeConfig that is optional in the main config, but with both the API key and url being required within the model itself. That would mean we don't need custom validation since Pydantic will allow nothing to be provided (i.e. user_office_config=None) or both, but if only one is provided then `UserOfficeConfig is missing a required field and it will fail.
There was a problem hiding this comment.
Done. There are a few files changed, but they're mostly single-line changes. I had forgotten we could check on the empty tag, and we do this elsewhere, so this is a lot neater. Good catch.
|
I initially assumed a sub-model would require moving the User Office settings out of the auth section, but I suppose it can be an optional nested model within AuthConfig, so a sub sub model. So the config would look like: So the user office section is optional, and other deployments just won't have. Sound ok? |
This PR:
Adds string as a new data type:
Introduces string as a supported data type.
Allows string for shotnum field:
Updates the shotnum field to accept both int and string values.
Maintains backward compatibility with existing numeric shot numbers.
Add an optional type field to shotnum:
Introduces an optional type attribute to the model for shotnum datatype.
If type is not provided, the system infers the type based on the value (str or int).
Support sub-second timestamp ingestion:
Extends ingestion logic to handle timestamps with sub-second precision.
Enhancements to range converter:
Updates range conversion logic to correctly handle mixed shotnum types (int and string).
Clean up:
Adds a new lock file for dependency updates (which is the bulk of the reported line changes)