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

document the use of entry_points to register new backends #548

Merged
merged 3 commits into from
Mar 2, 2021

Conversation

d70-t
Copy link
Contributor

@d70-t d70-t commented Feb 27, 2021

I just found out that registering new backends is now possible using entry_points, thanks to #515. I think that this feature needs at least some basic documentation, so this is what this PR is for.

One think to note is that it actually works a little different from what is written in #515:

This is from the initial message in 515

   entry_points=[
       'fsspec.specs': [
           'myfs=myfs:MyFs',
       ]
   ],

and this is what actually works:

    entry_points={
        'fsspec.specs': [
            'myfs=myfs.MyFS',
        ],  
    },

The change of [] to {} seems to be a typo, but I am not sure if that is the case for : -> .. According to the setuptools manual, the syntax for defining entry points is:

<name> = [<package>.[<subpackage>.]]<module>[:<object>.<object>]

and thus the original comment in #515 would be correct in this regard, but the code is maybe wrong.


This PR documents what currently works. But probably we should change the code and the docs such that the filesystem object will be separated by a : in the entry_points.

@d70-t
Copy link
Contributor Author

d70-t commented Feb 27, 2021

While implementing this for an own backend (fsspec/ipfsspec#2), I discovered that this is not working in Python 3.8 as in that version importlib.metadata is already available, but its EntryPoint object does not yet have a module attribute. As this concerns exactly the line which probably must be changed regarding the : issue, I'd change this PR into a draft for now and ask how we might proceed?

@d70-t d70-t marked this pull request as draft February 27, 2021 14:37
@d70-t
Copy link
Contributor Author

d70-t commented Feb 27, 2021

Currently I think of two possible ways to solve this:

  • Replace spec.module by spec.value.replace(':', '.') in __init__.py: seems to be a bit hackish, but should work for both, the currently implemented variant as well as if : is used in entry_points. Note that .value is available in Python 3.8.
  • Use the entrypoints library as intake does: seems to be more proper, but pulls in an additional dependency and is more complicated

@martindurant
Copy link
Member

Given the current situation, we should probably support both "package.module.class" and "package.module:class", sorry. Should be simple

if ":" in spec:
     mod, class = spec.split(":", 1)
else:
    mod, class = spec.rsplit(".", 1)

@d70-t
Copy link
Contributor Author

d70-t commented Mar 1, 2021

If I get it right, at this place the only thing required would be a dotted string, without splitting it into mod and class. I've added this changed to the PR for illustration. I think it would work like this, that is either : or . would be supported.

There's however one more thing which bothers me. If any backend library which uses the fsspec.specs entry point is installed in a python 3.8 environment and a version of fsspec which does not include this PR is installed, then a simple

import fsspec

will crash. We could either not bother and hope that prior to a wider adoption of the entry point, users will have upgrade the fsspec version or change the name of the entry point.

@martindurant
Copy link
Member

I don't see a problem with the current implementation here. If someone follows the cods, it will work, and if someone uses the entrypoints convention, it will also work. Is there any case where the part after the ":" might be more complex? I don't think so - or at least, we don't need to support it.

@d70-t
Copy link
Contributor Author

d70-t commented Mar 2, 2021

I also don't see a problem with this implementation. According to the setuptools manual, there will only be .-separated objects after the :.

Should I update the docs to recommend : then, to be in accordance with setuptools?

@martindurant
Copy link
Member

Should I update the docs to recommend : then, to be in accordance with setuptools?

I don't think so, because the code doesn't actually handle it - it will attempt to import the whole string before the final ".". i.e., just because you can do import mod; mod.classy.objecty, doesn't mean you can do import mod.classy.

It would be reasonable to respect the exact intention of ":" in the given string, if it's there.

@d70-t d70-t marked this pull request as ready for review March 2, 2021 18:27
@d70-t
Copy link
Contributor Author

d70-t commented Mar 2, 2021

Ok, that's reasonable. I'll leave it as is.

@martindurant martindurant merged commit 68aef2d into fsspec:master Mar 2, 2021
@d70-t d70-t deleted the doc_entry_points branch March 2, 2021 18:29
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.

None yet

2 participants