Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: move cave secrets from secrets to cave_secrets #528

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

william-silversmith
Copy link
Contributor

@william-silversmith william-silversmith commented Feb 8, 2022

This will allow specifying the storage secrets and the CAVE
secret at the same time.

This will allow specifing the storage secrets and the CAVE
secret at the same time.
@fcollman
Copy link
Collaborator

fcollman commented Feb 8, 2022

can we do this is a deprecation warning kind of way and accumulate the breaking changes for a future major version change?

@fcollman
Copy link
Collaborator

fcollman commented Feb 8, 2022

so for now as a patch change, add a deprecation warning that you should pass secrets in this other keyword argument, but still accept them in this one for graphene, then move the cleaner code version to a major version branch.

@william-silversmith
Copy link
Contributor Author

Yea that's no problem to keep it backwards compatible. I only considered doing it as breaking because there appear to be no references to secrets in PCG. I'll add in the shim.

@william-silversmith
Copy link
Contributor Author

It looks like the only code within the seung-lab github that would eventually need to be updated is this line: https://github.com/seung-lab/CAVEclient/blob/42e38a4fb8e0e73d8a911ca9edea18840f9ca786/caveclient/infoservice.py#L387

Not sure about other orgs though.

@ceesem
Copy link
Collaborator

ceesem commented Feb 8, 2022

That code was just added (partially to deal with a case like this) a day or two ago and is not in use in the world outside a few of my notebooks. Every time any code anywhere else is generating a cloudvolume — from both analysis scripts and services — it has to use the current secrets argument through cloudvolume explicitly. The number of users who would be affected would span not only services but a significant fraction of people doing analysis or guidance scripting as well. This is a very significant breaking change and shouldn't be made lightly, even though I understand the hypothetical problem it solves.

@ceesem
Copy link
Collaborator

ceesem commented Feb 8, 2022

For example, you could make a new parsing system that lets you specify secrets explicitly based on prefix, e.g.:

cloudvolume.Cloudvolume(..., secrets={'graphene': cave_token, 'gs': google_secret})

that would resolve the ambiguity in the case you described but allow the current pattern to work while allowing the less verbose current nomenclature to persist during a period of deprecation.

@william-silversmith
Copy link
Contributor Author

Referencing issue #453

That makes sense to me and the design would accommodate growth. My goal is to always present as (seemingly) simple an interface as possible, so the old way will hopefully never be deprecated since that would impose a cost on every user since the most common use case is surely accessing raw precomputed from all kinds of datasets.

I think you're probably right that there are a few people using secrets as a parameter, but github search seems find few examples across all public codebases. My feeling is they probably mostly hiding in one-off scripts and Jupyter notebooks.

https://github.com/search?l=Python&p=1&q=CloudVolume+secrets&type=Code

@jefferis
Copy link

jefferis commented Feb 8, 2022

I haven’t thought through the implications of this change but just for completeness https://github.com/search?l=R&q=CloudVolume+secrets&type=Code.

@william-silversmith
Copy link
Contributor Author

Thought about this some more. I don't think Casey's suggestion will work because the secrets you input are already dicts. I think cave_secret (with backwards compatibility) is fine though perhaps a more generic name can be chosen to anticipate other use cases.

@william-silversmith william-silversmith changed the title BREAKING: feat: move cave secrets from secrets to cave_secrets feat: move cave secrets from secrets to cave_secrets May 6, 2022
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.

4 participants