-
Notifications
You must be signed in to change notification settings - Fork 24
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
Conda Fetch Plugin Hook #44
base: main
Are you sure you want to change the base?
Conda Fetch Plugin Hook #44
Conversation
Co-authored-by: Bianca Henderson <beeankha@gmail.com>
This is more or less ready for review. I still need to add some FAQs and there are probably grammar errors, but the spirit of what I want to convey is there.
…away/ceps into cep-conda-session-pluing-hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the hook has me worried here, it's a little Russian doll style naming confusion IMO and visible in the example: CondaSessionClass
has a session_class
attribute of subclasses of CondaSession
.
I wonder if we should use "session class" for the plugin name, as it's an implementation detail of the requests library and not a feature of conda itself. It feels too overused for a plugin hook and I think precludes what this could be used for.
I think a few things could work to balance this out:
CondaSession
should be renamed toCondaRequestsSession
since that's what it is and would prevent 3rd party devs to accidentally try to mimic that class instead of therequests.Session
class.- Use "requester" or "transmitter" as the domain name of this new plugin hook.
So thatconda install --transmitter=pycurl scipy
works
The example would look like this:
class CustomCondaRequestsSession(CondaRequestsSession):
"""
Our custom CondaSession class which defines an additional class
property
"""
def __init__(self):
super().__init__()
self.custom_property = 'custom_property'
@hookimpl
def conda_transmitters():
"""
Register our custom CondaRequestsSession class
"""
yield CondaTransmitter(
name=PLUGIN_NAME,
session=CustomCondaRequestsSession
)
The UX would be:
$ CONDA_TRANSMITTER=pycurl conda install scipy
$ conda install --transmitter=pycurl scipy
new-cep.md
Outdated
Those are just a few examples of authentication schemes this plugin | ||
would enable conda to support. Because there are many more, it would be | ||
unfeasible and difficult to maintain each and every one if we decided to | ||
add these all as custom authentication schemes to conda itself. This is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may also have to implement entirely new authentication schemes in the future, that currently don't exist.
new-cep.md
Outdated
|
||
## FAQs | ||
|
||
_Any FAQs will go here. Plus add any in the comments of the pull request!_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- "Why did you choose to use requests' Session class as the basis for this plugin hook?"
- "What are the expected behavior inherent 3rd party conda session classes?" (e.g. thead-safety etc)
BTW as some inspiration for people creating requests-style session classes: https://github.com/dcoles/pycurl-requests |
Co-authored-by: Jannis Leidel <jannis@leidel.info>
I really like your "transmitter" idea. You pointed something out that I felt more and more awkward about as I was writing the plugin prototype, which is the naming of the Also, thank you for adding those high level UX examples. I will definitely make room for those in the CEP too (under "Specification"). |
…to type 😉; I think this is also what we originally wanted to call this
I switched this back to |
new-cep.md
Outdated
```yaml | ||
channels: | ||
- https://my-custom-conda-packages.com: | ||
fetch: custom_fetch | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can this be achieved via the CLI or env vars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure there is a clean way to do this that would be the most flexible.
Here's an example of a method one could use on the command line to effectively achieve the the configuration defined above:
conda install python \
--channels "https://my-custom-conda-packages.com" \
--fetch custom_fetch \
--override-channels
and with environment variables:
export CONDA_CHANNELS="https://my-custom-conda-packages.com"
export CONDA_OVERRIDE_CHANNELS=1
export CONDA_FETCH=custom_fetch
conda install new_package
The only problem is that this approach breaks as soon as you want to add another channel in there (e.g. "conda-forge"). For the most flexible option, I think passing in JSON as the value for channel makes the most sense:
conda install python \
--channels '{"https://my-custom-conda-packages.com": {"fetch": "custom_fetch"}}'
and as an environment variable:
export CONDA_CHANNELS='{"https://my-custom-conda-packages.com": {"fetch": "custom_fetch"}}'
That is definitely not the prettiest approach, but it Works™️. It lines up with changes that we are currently proposing for channel configuration (conda/conda#12033).
Do you have any other ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, after some thinking I think we would need JSON support for maps. I don't know if that can become a problem by being too flexible, though. Nothing Pydantic validation and schemas cannot help with :D
Co-authored-by: Jaime Rodríguez-Guerra <jaimergp@users.noreply.github.com>
I wonder if we should be including support for adding additional adapters here too: Perhaps it makes sense to be able to expose just these as hooks too. |
I would be interested in this. We ended up creating a mock object for our session purposes. It would be nice to move to something more supported by the community. with mock.patch('conda.gateways.connection.session.CondaSession',
return_value=CondaSessionRSA()): |
We recently had a new feature request in conda repository that could also be supported by this new plugin hook: This would also be a good case for allowing plugin authors for adding additional adapters. |
This CEP proposed a new plugin hook that will allow plugin authors to completely customize the way that network requests are made. Please check what is currently on the CEP and use this pull request as a forum to discuss changes or oppositions to it.