-
Notifications
You must be signed in to change notification settings - Fork 2
Adds support for IPL-4 #85
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
Conversation
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.
Pull Request Overview
This PR adds support for IPL-4 by implementing dynamic Astra schema selection based on data release versions. The key changes include updating schema mapping logic to handle the new IPL-4 version (0.8.0) while maintaining backward compatibility with DR19/IPL-3 (0.5.0, 0.6.0).
- Implemented dynamic Astra schema selection in database connection logic
- Enhanced query functions with proper documentation and simplified logic
- Updated data model to make certain fields optional for compatibility
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/valis/db/db.py | Added dynamic Astra schema selection based on release version in database connection dependency |
| python/valis/db/queries.py | Simplified Apogee target retrieval, added comprehensive docstrings, and enhanced schema validation |
| python/valis/db/models.py | Made several AstraSource fields optional to accommodate schema variations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
python/valis/db/queries.py
Outdated
| if not vastra or vastra not in ("0.5.0", "0.6.0"): | ||
| print('astra only supports DR19 / IPL3 = version 0.5.0, 0.6.0') | ||
| vastra = "0.5.0" if vastra in ("0.5.0", "0.6.0") else vastra | ||
| if vastra.replace('.', '') not in astra.Source._meta.schema: |
Copilot
AI
Oct 23, 2025
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.
The condition vastra.replace('.', '') not in astra.Source._meta.schema will fail when vastra is None (returned by get_software_tag when release is invalid). This will cause an AttributeError. Add a check for None before calling .replace().
| if vastra.replace('.', '') not in astra.Source._meta.schema: | |
| if vastra is None or vastra.replace('.', '') not in astra.Source._meta.schema: |
| vastra = "0.5.0" if vastra in ("0.5.0", "0.6.0") else vastra | ||
| schema = f"astra_{vastra.replace('.', '')}" |
Copilot
AI
Oct 23, 2025
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.
[nitpick] The version mapping logic vastra = \"0.5.0\" if vastra in (\"0.5.0\", \"0.6.0\") else vastra is duplicated in both get_pw_db and get_astra_target. Consider extracting this logic into a shared utility function to improve maintainability.
This PR adds support for IPL-4 into valis.