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

Using entry points for loading backends #2379

Merged
merged 26 commits into from
Feb 25, 2021
Merged

Using entry points for loading backends #2379

merged 26 commits into from
Feb 25, 2021

Conversation

datapythonista
Copy link
Contributor

With entry points, the code is not only cleaner, but this will allow that third-party backends are used in the same exact way as packages deployed with Ibis. For example:

import ibis

ibis.mssql.connect(...)

This will work if ibis-mssql is installed in the environment.

This change also allows to easily move backends out of Ibis, for example OmniSci (see #2356). Having the backend outside of this repo will be backward compatible and no change to the user code will be needed.

@datapythonista datapythonista added refactor Issues or PRs related to refactoring the codebase backends Issues related to all backends labels Sep 14, 2020
@jreback jreback added this to the Next Bugfix Release milestone Sep 16, 2020
@jreback
Copy link
Contributor

jreback commented Sep 16, 2020

can you rebase

@datapythonista
Copy link
Contributor Author

There is something very strange going on here. Apparently, in some parts of the code, when using import ibis; ibis.<backend>, instead of calling __getattr__ of ibis/__init__.py, the result is the module ibis/<backend>/__init__.py.

I think the responsibly is pytest. I'm not sure, but I think it does a lot of Python internal monkeypatching, and that's the only reason I can find.

Regardless of this error, I don't think it's a great idea that ibis.pandas refers to ibis/pandas/api.py and not the ibis/pandas module. Quite misleading. I'll do a test and see if moving one of the affected backends solves the problem (ibis/pysparks -> ibis/backends/pyspark/). If it does, I'll start moving the backends to that directory. Which I think makes a lot of sense anyway, and I was planning to do at some point.

@jreback
Copy link
Contributor

jreback commented Sep 16, 2020

There is something very strange going on here. Apparently, in some parts of the code, when using import ibis; ibis.<backend>, instead of calling __getattr__ of ibis/__init__.py, the result is the module ibis/<backend>/__init__.py.

I think the responsibly is pytest. I'm not sure, but I think it does a lot of Python internal monkeypatching, and that's the only reason I can find.

Regardless of this error, I don't think it's a great idea that ibis.pandas refers to ibis/pandas/api.py and not the ibis/pandas module. Quite misleading. I'll do a test and see if moving one of the affected backends solves the problem (ibis/pysparks -> ibis/backends/pyspark/). If it does, I'll start moving the backends to that directory. Which I think makes a lot of sense anyway, and I was planning to do at some point.

yep this all sounds good.

@jreback
Copy link
Contributor

jreback commented Sep 21, 2020

@datapythonista looks like needs a rebase.

@datapythonista
Copy link
Contributor Author

@datapythonista looks like needs a rebase.

Rebased. Let's see if moving the omnisci backend fixed the strange pytest error. If that's the case, we'll still have to move the spark backends (its CI was also red).

@jreback
Copy link
Contributor

jreback commented Sep 21, 2020

dont' we need to rebuild the actual conda packages first?

setup.py Outdated
entry_points={
'ibis.backends': [
'pandas = ibis.pandas.api'
'csv = ibis.file.csv',
Copy link
Contributor

Choose a reason for hiding this comment

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

i think if you sync this now ,then the updates for the other backends can update this as needed

@datapythonista datapythonista removed this from the Next Bugfix Release milestone Nov 11, 2020
@datapythonista datapythonista added this to the Next release milestone Nov 13, 2020
@datapythonista datapythonista changed the title Using entry points for loading backends WIP: Using entry points for loading backends Dec 1, 2020
@datapythonista datapythonista changed the title WIP: Using entry points for loading backends Using entry points for loading backends Jan 14, 2021
@pep8speaks
Copy link

pep8speaks commented Feb 24, 2021

Hello @datapythonista! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-02-24 19:23:33 UTC

@datapythonista
Copy link
Contributor Author

@jreback this is green now, and should be ready.

@jreback
Copy link
Contributor

jreback commented Feb 24, 2021

cool, this doesn't actually change the user behavior right?

@datapythonista
Copy link
Contributor Author

cool, this doesn't actually change the user behavior right?

Not at all. It's just an internal change that lets third party packages to also register backends. User API is exactly the same, that's why there are no changes to tests and all is green.

@jreback jreback merged commit f22e79f into ibis-project:master Feb 25, 2021
@jreback
Copy link
Contributor

jreback commented Feb 25, 2021

great and actually we dont' import things pre-emptively now, so import times should be faster?

@datapythonista
Copy link
Contributor Author

great and actually we dont' import things pre-emptively now, so import times should be faster?

We still load all backends at init unfortunately. I wanted to do that, but the problem are the backend specific options. If the backend is loaded when we do ibis.sqlite, that's when the options would be loaded, and if you set them before with a with for example, that would fail.

I think we can find a solution, with small API changes, but will come later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backends Issues related to all backends refactor Issues or PRs related to refactoring the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants