Improve handling of unsorted VCF files #636
Conversation
src/tiledb/cloud/vcf/ingestion.py
Outdated
| uri = sort_and_bgzip(uri, tmp_space=tmp_space) | ||
| tmp_uris.append(uri) | ||
| create_index_file(uri) | ||
| except Exception as e: |
There was a problem hiding this comment.
@gspowley is it possible to catch only the one (I assume? Could be wrong) specific exception that occurs in create_index_file()?
There was a problem hiding this comment.
Most likely, I'll have to check.
| create_index_file(uri) | ||
| except Exception as e: | ||
| logger.error("%r: %s", uri, e) | ||
| return None |
There was a problem hiding this comment.
Should we be re-raising here instead of returning?
There was a problem hiding this comment.
In this case, we want to log the error and proceed. This allows the ingestion to proceed with the good VCF files.
|
The change to |
I wasn't aware of that PR, but yes the changes do overlap. I recommend we merge this PR first, since it's making functional changes to the logic. |
| yield None | ||
|
|
||
| logger.info(f"Removing {_LOCAL_WHEEL}") | ||
| os.remove(_LOCAL_WHEEL) |
There was a problem hiding this comment.
@gspowley I think it would be better if we didn't write to our project source during testing. One less thing to worry about if anything goes wrong, is what I'm thinking. How about copying to the built-in pytest tmp_path and then re-pointing _LOCAL_WHEEL at that?
I might attack the use of globals to pass these parameters later, after we get ourselves out of this jam.
| global _LOCAL_WHEEL | ||
|
|
||
| tmp_path = tmp_path_factory.mktemp("wheel") | ||
| _LOCAL_WHEEL = os.path.join(tmp_path, os.path.basename(_LOCAL_WHEEL)) |
There was a problem hiding this comment.
Great! Thanks for accepting my suggestion @gspowley , that's all from me.
sort/bgzip/indexby using oneThreadPool.Non-related CI changes:
.test_durationsand avoid error splitting pyteststest_wheel.pyto avoid failures due to collisions between concurrent tests.