pangeo-forge/pangeo-forge-recipes

`write_combined_reference` on `main` doesn't reuse dep injected `FSSpecTarget` credentials

ranchodeluxe opened this issue · 4 comments

ranchodeluxe commented

Problem

There seems to be a regression on main compared to 0.10.4 release where write_combined_reference doesn't use the dep injected credentials from FSSpecTarget.

I see a thread in the merged PR for this regression there was some confusion about this and that target_options and remote_options was a work around.

Possible Solution

In 0.10.4 this context manager block was doing the magic. I assume we can add that back and remove all the remote_options and target_options in favor of dep injection just like StoreToZarr works? Thoughts?

ranchodeluxe commented

Additional notes if using s3 or google storage:

You wouldn't run across this issue on LocalDirectBakery if you happen to have global os environment AWS_* credentials exported and they just happen to grant you access to the same TargetStore as your runner config. They are inherited by the worker processes from the parent runner process. So fsspec will discover these global credentials and use them probably before introspecting the dep injected ones

norlandrhagen commented

Great digging @ranchodeluxe! Do you want to open a PR fix for this or would you prefer to review?

In summary for the fix:

  • Re-implement this context manager.
  • tear out any mention of remote_options and target_options.

We have those minio integration tests for AWS recipes. Are there any global os environment AWS* credentials_ that need to be added/updated to the minio config for this to work?

ranchodeluxe commented

Great digging @ranchodeluxe! Do you want to open a PR fix for this or would you prefer to review?

In summary for the fix:

* Re-implement this [context manager](https://github.com/pangeo-forge/pangeo-forge-recipes/blob/0.10.4/pangeo_forge_recipes/writers.py#L123).

* tear out any mention of `remote_options` and `target_options`.

I didn't know I had options! I can put in a fix tomorrow

We have those minio integration tests for AWS recipes. Are there any global os environment AWS* credentials_ that need to be added/updated to the minio config for this to work?

Our integration tests only apply this via config and dep injection so we should be safe 👍

  • 👍1
ranchodeluxe commented

already folded into @main and working so closing