Skip to content

issue remove check for s3:// in href for setting output_dir#1738

Open
ElienVandermaesenVITO wants to merge 5 commits into
masterfrom
remove-check-for-s3
Open

issue remove check for s3:// in href for setting output_dir#1738
ElienVandermaesenVITO wants to merge 5 commits into
masterfrom
remove-check-for-s3

Conversation

@ElienVandermaesenVITO

Copy link
Copy Markdown
Contributor

@ElienVandermaesenVITO

Copy link
Copy Markdown
Contributor Author

I removed the not asset["href"].startswith("s3://"): from

if "output_dir" not in asset and not asset["href"].startswith("s3://"):
but I'm not certain the job_dir is the right one. It need to be something like s3://openeo-data-prod-otc-waw4-1/batch_jobs/j-2607020759334106be8a957ca485260f

Comment thread tests/test_backend.py Outdated
"id": "b9510f20-92a0-4947-a4b6-a6a934a0015e",
"assets": {"openEO": {"href": "s3://bucket/path/to/openEO.tif"}},
"assets": {"openEO": DictSubSet({
"href": "s3://bucket/path/to/openEO.tif",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not make it explicit what the output_dir is in this test?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the failed test I see

                'openEO': {
                    'href': 's3://bucket/path/to/openEO.tif',
  +                 'output_dir': '/batch_jobs/j-260702134802413cb2c8fcf711657072',
                },

and you know the job id at that point, so you could just build the expected output dir

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not make it explicit because I thought it was not relevant to the test, but it is also a good option.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But do you know the /openeo-data-prod-otc-waw4-1 part, because there are multiple options?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But do you know the /openeo-data-prod-otc-waw4-1 part, because there are multiple options?

that's not relevant for this PR, right? Or am I misunderstanding?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The href is something like:
https://openeo.dataspace.copernicus.eu/openeo/1.2/jobs/j-2607020759334106be8a957ca485260f/results/assets/Zm...A5/c9...c4/s3://openeo-data-prod-otc-waw4-1/batch_jobs/j-2607020759334106be8a957ca485260f/openEO.tif?expires=1783669484,
what you want is
https://openeo.dataspace.copernicus.eu/openeo/1.2/jobs/j-2607020759334106be8a957ca485260f/results/assets/Zm...A5/c9...c4/openEO.tif?expires=1783669484
That means that s3://openeo-data-prod-otc-waw4-1/batch_jobs/j-2607020759334106be8a957ca485260f is too much, this has to be put into the output_dir and is filtered out later. So /openeo-data-prod-otc-waw4-1 is also relevant here.

@soxofaan soxofaan Jul 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but I think that "prefix" is going to be removed automatically by the current "commonpath/relpath" logic at https://github.com/Open-EO/openeo-python-driver/blob/ce77ac84bfffdca3feb7aba450c6a7f1609c07a5/openeo_driver/views.py#L1253-L1257
so that's why I don't think it's relevant for this particular PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants