feat: add field parameters transfer and associated metrics handling #423
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for transferring field parameters data by introducing a new FieldParametersTransferer class and integrating it into the existing transfer pipeline with associated metrics tracking.
Changes:
- Added
transfer_field_parametersconfiguration option with environment variable support - Integrated field parameters transfer into both parallel and sequential transfer execution paths
- Added metrics handling for field parameters and NMA stratigraphy data
- Renamed "Stratigraphy" to "StratigraphyLegacy" for clarity in parallel transfer execution
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| transfers/transfer.py | Added field parameters transfer configuration, import, and execution in both parallel and sequential paths; renamed stratigraphy label |
| transfers/metrics.py | Added metrics methods for field parameters and NMA stratigraphy with appropriate model imports |
| from typing import Sequence, Union | ||
|
|
||
| from alembic import op |
There was a problem hiding this comment.
The migration file has removed imports for geoalchemy2, sqlalchemy, and sqlalchemy_utils. If these imports were removed because they're unused, this is good. However, verify that the migration still functions correctly without these dependencies.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 129668e75b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| future = executor.submit( | ||
| _execute_transfer_with_timing, | ||
| "Stratigraphy", | ||
| "StratigraphyLegacy", |
There was a problem hiding this comment.
The display name "StratigraphyLegacy" is inconsistent with the actual transferer class name StratigraphyLegacyTransferer and doesn't match the pattern used elsewhere (e.g., "WaterLevelsPressureDaily" for NMA_WaterLevelsContinuous_Pressure_DailyTransferer). Consider using a name that better reflects the model being transferred, such as "NMA_Stratigraphy" to maintain consistency with other legacy table names.
chasetmartin
left a comment
chasetmartin
left a comment
There was a problem hiding this comment.
tests are passing now with a couple name fixes
Why
This PR addresses the following problem / context:
How
Implementation summary - the following was changed / added / removed:
Notes
Any special considerations, workarounds, or follow-up work to note?