search

mobie / mobie-utils-python

Python tools for MoBIE
MIT License
9 stars 5 forks source link

add remote support for cross-dataset sources #114

Closed martinschorb closed 9 months ago

martinschorb commented 10 months ago

This adds support for generating remote paths to sources originating from another dataset in the same project.

martinschorb commented 10 months ago

tests are still missing. My idea is to create another "test-raw" dataset and refer to its sources in test_remote_metadata and then check the generated URLs.

tischi commented 10 months ago

@martinschorb could you post an example here for how such a relative remote path would look like? And I guess those paths would be the ones stored in the dataset.json, such as here?

martinschorb commented 10 months ago

One example is here:

https://github.com/mobie/environmental-dinoflagellate-atlas/blob/a6a7cab0baeaa8b9a0290ce94b5f67f25775e672/data/all_years/dataset.json#L1336

tischi commented 10 months ago

Thanks @martinschorb, did you try already whether that works with the current MoBIE?

martinschorb commented 10 months ago

Yes, this project is fully functional and validates correctly using mobie-python. I actually used my code from this PR to generate it. Just realized that it will not work on Windows \paths hooray, but am working on fixing that.

martinschorb commented 10 months ago

OK, just tested on Windows and it works out of the box, even without adjustment ;-)

constantinpape commented 9 months ago

Hi @martinschorb, this looks good overall and the use-case makes sense, great that it works already!

tests are still missing. My idea is to create another "test-raw" dataset and refer to its sources in test_remote_metadata and then check the generated URLs.

Do you still want to create tests before merging? I think this would be a good idea, but if you need this added now we could also go ahead without tests.

martinschorb commented 9 months ago

Do you still want to create tests before merging? I think this would be a good idea, but if you need this added now we could also go ahead without tests.

Working on the tests, I realized, that add_source_to_dataset resolves the paths of "other" datasets as relative paths. However, I could not find tests for this behavior. Since my test of the remote paths now relies on this functionality, we might need to add this upstream test as well.

martinschorb commented 9 months ago

Any objections before merging? I don't know why MacOS fails during the minconda setup but it is not due to any code changes but some CI problem.

constantinpape commented 9 months ago

Any objections before merging?

Sorry I didn't have time to check this out yet, I am pretty busy these days so it takes a few days for me to answer. I think this is ok, but I want to check what you changed in the tests more carefully.

I don't know why MacOS fails during the minconda setup but it is not due to any code changes but some CI problem.

Yeah, this is an unrelated issue. I will update the CI to micromamba, which is faster and more reliable.