Add logic to copy historical data - #2187
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to ingest historic data from GCS into the versioned output directory, controlled by a new configuration flag ingest_historic_data. However, the changes also comment out the virtual environment creation (_create_venv), replacing it with a dummy process and sys.executable, which will bypass dependency installation and likely cause runtime failures. Additionally, the GCS blob listing logic for historic data should be refined to use a slash-terminated prefix to prevent matching unintended directories and to simplify path parsing.
| storage_client = storage.Client(self.config.gcs_project_id) | ||
| bucket = storage_client.bucket( | ||
| self.config.storage_prod_bucket_name) | ||
| source_prefix = f'{output_dir}/historic/' |
There was a problem hiding this comment.
Wondering if it would be safer to list specific files in GCS to be used in the manifest.json rather than picking up all files. This would make reviews easier, o/w copy to historic folder will have to wait until the update is reviewed.
On the flip side, this will add some overhead of managing the list through manual PRs rather than automatic simple file copy.
| if import_summary: | ||
| import_summary.import_stats['historic_data_size'] = historic_data_size | ||
| import_summary.data_volume += historic_data_size | ||
| logging.info( |
There was a problem hiding this comment.
Can we also log the number and names of files that were copied?
| import_summary.import_stats.get('validation_data_size', 0)) | ||
| logging.info(import_summary) | ||
|
|
||
| if self.config.ingest_historic_data: |
There was a problem hiding this comment.
Can this be a list of files rather than a flag? If non-empty, then files are copied.
| storage_client = storage.Client(self.config.gcs_project_id) | ||
| bucket = storage_client.bucket( | ||
| self.config.storage_prod_bucket_name) | ||
| source_prefix = f'{output_dir}/historic/' |
There was a problem hiding this comment.
instead of hardcoding the folder '/historic/', can this be a parameter that defaults to /historic/?
| if blob.size: | ||
| historic_data_size += blob.size | ||
| except Exception as e: | ||
| logging.error(f'Failed to copy historic data from GCS: {e}') |
There was a problem hiding this comment.
pls log the src and dst file names as well.
2 participants
No description provided.