Issue/324 manger test #362
Conversation
|
Tests pass - locally - strange .... |
JackKelly
left a comment
JackKelly
left a comment
There was a problem hiding this comment.
Looks good!
A few minor comments...
If you're using logger.warning so the log messages appear during unittests then I think there is a way to set the log level to, say, debug during tests (but I can't remember exactly how to do that!)
| # Split locations per example into batches: | ||
| n_batches = len(spatial_and_temporal_locations_of_each_example) // batch_size | ||
| locations_for_batches = [] | ||
| logger.warning("xxxxx") |
There was a problem hiding this comment.
Is this warning still required? 🙂
| n_batches = len(spatial_and_temporal_locations_of_each_example) // batch_size | ||
| locations_for_batches = [] | ||
| logger.warning("xxxxx") | ||
| logger.warning(n_batches) |
There was a problem hiding this comment.
Please make this warning a bit more verbose?
| for n_batches_processed, locations_for_batch in enumerate(locations_for_batches): | ||
| batch_idx = idx_of_first_batch + n_batches_processed | ||
| logger.debug(f"{self.__class__.__name__} creating batch {batch_idx}!") | ||
| logger.warning(f"{self.__class__.__name__} creating batch {batch_idx}!") |
There was a problem hiding this comment.
Sorry if I've misunderstood but why does this message need to be a warning?
| # Save batch to disk. | ||
| netcdf_filename = path_to_write_to / nd_utils.get_netcdf_filename(batch_idx) | ||
| batch.to_netcdf(netcdf_filename) | ||
| logger.warning(f"Save file to {netcdf_filename}") |
There was a problem hiding this comment.
Same as above: Does this message really need to be a warning?
| filesystem = get_filesystem(dst_path) | ||
| filesystem.put(str(local_path), dst_path, recursive=True) | ||
|
|
||
| _LOG.warning(f"moving files from {local_path} to {dst_path}") |
There was a problem hiding this comment.
Does this need to be a warning?
|
|
||
| _LOG.warning(f"moving files from {local_path} to {dst_path}") | ||
|
|
||
| _LOG.warning(get_all_filenames_in_path(local_path)) |
There was a problem hiding this comment.
Please make these two messages a bit more verbose (or maybe remove these two messages?) Also, do these need to be warnings?
| filesystem.put(str(local_path) + "/", str(dst_path) + "/", recursive=True) | ||
| delete_all_files_in_temp_path(local_path) | ||
|
|
||
| _LOG.warning(get_all_filenames_in_path(local_path)) |
| data_source.create_batches, | ||
| print(executor) | ||
| # future = executor.submit( | ||
| logger.warning("Making batches") |
There was a problem hiding this comment.
Do these need to be warnings?
| black | ||
| pre-commit | ||
| fsspec | ||
| fsspec==2021.7.0 |
There was a problem hiding this comment.
Ooh, interesting, are we sure we can't use more recent versions of fsspec? A few days ago, there was a bug (I think) in fsspec which forced pip to install an ancient version of gscfs but that appears fixed now (if that's what make you pin fsspec to version 2021.7.0?
|
Hey @JackKelly sorry i closed this PR and move to #363 I was debugging using CI as tests worked on my laptop. It turns out the fix was to use |
Pull Request
Description
add tests for manager
Want to do this before #341
Fixes #324
How Has This Been Tested?
normal unit test
Checklist: