changes from manual testing #81
Conversation
| options.urls = { | ||
| "datasets" : urljoin(options.host, "datasets"), | ||
| "proposals" : urljoin(options.host, "proposals"), | ||
| "origdatablocks" : urljoin(options.host, "origdatablocks"), | ||
| "instruments": urljoin(options.host, "instruments"), | ||
| } |
There was a problem hiding this comment.
These 4 endpoints don't have to be configurable from the config file, no...?
Can I turn them to be a property instead?
| if __name__ == "__main__": | ||
| main() |
There was a problem hiding this comment.
We don't need it cause we expose main function as scicat_ingestor command.
You can just type scicat_ingestor to start the online ingestor.
(it doesn't hurt to have it though)
| if __name__ == "__main__": | ||
| main() |
There was a problem hiding this comment.
Also these. You can just type background_ingestor instead of using the file itself as a script.
| # if file_path.exists() and compute_file_stats: | ||
| # return DataFileListItem( | ||
| # path=file_path.absolute().as_posix(), | ||
| # size=(file_stats := file_path.stat()).st_size, | ||
| # time=datetime.datetime.fromtimestamp( | ||
| # file_stats.st_ctime, tz=datetime.UTC | ||
| # ).strftime("%Y-%m-%dT%H:%M:%S.000Z"), | ||
| # chk=_calculate_checksum(file_path, file_hash_algorithm) | ||
| # if compute_file_hash | ||
| # else None, | ||
| # uid=str(file_stats.st_uid), | ||
| # gid=str(file_stats.st_gid), | ||
| # perm=oct(file_stats.st_mode), | ||
| # ) | ||
| # else: | ||
| # return DataFileListItem( | ||
| # path=file_path.absolute().as_posix(), | ||
| # time=datetime.datetime.now(tz=datetime.UTC).strftime( | ||
| # "%Y-%m-%dT%H:%M:%S.000Z" | ||
| # ), | ||
| # ) |
There was a problem hiding this comment.
I think the re-written part is fine. We can remove the commented out part...!
| @@ -38,7 +38,10 @@ def build_offline_config() -> OfflineIngestorConfig: | |||
| # with ``OnlineIngestorConfig``. | |||
| del merged_configuration["kafka"] | |||
|
|
|||
| return build_dataclass(OfflineIngestorConfig, merged_configuration) | |||
| config = build_dataclass(OfflineIngestorConfig, merged_configuration) | |||
| config.scicat = SciCatOptions.from_configurations(merged_configuration["scicat"]) | |||
There was a problem hiding this comment.
build_dataclass automatically builds the sub-dataclasses as well.
So it should be not needed.
If it wasn't it's a bug so I need to fix it...
| provided_path = path.split("/")[1:] | ||
| provided_path[0] = "/" + provided_path[0] |
There was a problem hiding this comment.
I think this part should be handled in the extract_paths_from_h5_file.
|
I'll just clone it and make the CI pass : D |
This PR includes all the changes that resulted from manual testing the offline ingestor.