Register ingested groups using the correct URI #679
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes the registration error in the directory case. Much less code and clearer about the intent of it.
sgillies
changed the title
pathlib loses the double slashes in our URIs, so we can't use that everywhere.
|
Directory ingestion works: https://beta.cloud.tiledb.com/taskgraphlogs/sean-gillies/dd293232-6537-44b5-aa20-9b7befc75787. Single file is succeeding, too: https://beta.cloud.tiledb.com/taskgraphlogs/sean-gillies/8f05b23d-e53b-41b7-a964-0ffe4159a69f. |
|
@johnkerl @aaronwolen can you check the soundness of my decision to normalize the ingestion outputs as I have done? |
sgillies
commented
Nov 12, 2024
sgillies
commented
Nov 12, 2024
sgillies
commented
Nov 12, 2024
sgillies
commented
Nov 12, 2024
sgillies
commented
Nov 12, 2024
| entry_output_uri = output_uri + "/" + base | ||
| if not output_uri.endswith("/"): | ||
| entry_output_uri += "/" | ||
| entry_output_uri += base |
There was a problem hiding this comment.
I don't understand why we appended base to the output path twice. Now we only do it once. If twice is a requirement, I can restore that behavior.
There was a problem hiding this comment.
I think it's not a requirement
johnkerl
approved these changes
Nov 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the directory ingestion case, the deployed code creates groups at
OUTPUT_URI/NAME/NAME/(note the doubledNAME) whereNAMEis the basename/stem of an H5ad file. But then it tries to register a group atOUTPUT_URI. There's no group there, so this fails.As a part of solving this problem, I'm generalizing directory ingest to cover the single file case. We no longer have two ingestion branches. There's much less code and I think the intent of the remaining code is more clear. This change helped me fix the bug and helps us make sure it stays fixed.
There's an output change, though: every H5ad file becomes a group at
OUTPUT_URI/NAME/whereNAMEis the basename/stem of the H5ad file no matter what. Ingested as a single file, or as an item in a folder, the same output. In theory (and practice, often) this kind of symmetry and de-specialization is a win. I'm going to need some review from actual SOMA users and developers, for sure.The test failures are unrelated. Trouble with task graph run times again.