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

Remove dependency on ipython_genutils. #622

Merged
merged 4 commits into from
Mar 14, 2021
Merged

Conversation

Carreau
Copy link
Member

@Carreau Carreau commented Mar 7, 2021

IPython Genutils seem to start to be troublesome to package for some Linux
Ditributions because of Python2/3 compat. Get rid of it.

It was not in the dependencies anyway...

@davidbrochart
Copy link
Member

LGTM

@Carreau
Copy link
Member Author

Carreau commented Mar 8, 2021

Thanks @davidbrochart

I'm going to get that in soon and do a release unless there are objections then.

@kevin-bates
Copy link
Member

I agree that the changes look good. My only question is what is the intention of this moving forward? Is the rest of the Jupyter stack supposed to remove what ipython_genutils references they can, then reply on jupyter_client for these "utils" methods? If so, these strike me as better-suited for jupyter_core.paths and, perhaps a new jupyter_core.import_string module. That is, let jupyter_core be the library rather than jupyter_client. I'm curious why client was picked over core - unless the intention is to let each package duplicate their own versions of the utilities.

What is the timeframe that the rest of the stack be updated?

@mcepl
Copy link

mcepl commented Mar 8, 2021

I agree that the changes look good. My only question is what is the intention of this moving forward? Is the rest of the Jupyter stack supposed to remove what ipython_genutils references they can, then reply on jupyter_client for these "utils" methods? If so, these strike me as better-suited for jupyter_core.paths and, perhaps a new jupyter_core.import_string module. That is, let jupyter_core be the library rather than jupyter_client. I'm curious why client was picked over core - unless the intention is to let each package duplicate their own versions of the utilities.

See ipython/ipython_genutils#17 and ipython/ipython#12840 ... aside from python-ipykernel I think we at least see the light in the end of the tunnel of converting whole ipython/jupyter universe from their own testing utilities and nose towards standard supported ones (unittest more or less sprinkled with pytest, some components heavily relying on it, some are pure unittest).

@Carreau
Copy link
Member Author

Carreau commented Mar 8, 2021

Yes, the point is for each package to duplicate version of the utilities if they need to; ipython_genutils is an artifact of The big split (2015) and the complexity at the time to support both Python 2 and 3 (as it predates six).

In many cases I even expect ipython_genutils to be replaced by no-op.

  • here I'm for example pretty certain that info['key'] will always be str,
  • most import logic can be replaced by imp/importlib. ( I expect most to actually be replaced by traitlets, as traitlet's Any, or DottedObjectName do the same with properties and import_item exists in traitlets.utils.importstring, though I didn't wanted to potentially introduce any differences, so haven't done that here).
  • Out of 250 imports in my dev folder 144 are ipython_genutils.py3compat (so can likely be removed)
  • many are nose decorators (skip_if_no_x11, skip_if_not_win32, that will need removal/update as nose will AFAICT not be compatible Python 3.10).
  • Other misc things that are now part of Core CPython.

@kevin-bates
Copy link
Member

Thank you for the responses @mcepl and @Carreau - they were helpful. I agree that most will fall out.

My comment was more about the methods added to utils - which will be imported into downstream packages. We had this same issue with secure_write (moved from N locations into jupyter_core) and it was painful. I'm just trying to ensure the same doesn't happen here.

import_item exists in traitlets.utils.importstring, though I didn't wanted to potentially introduce any differences, so haven't done that here

Thanks for the hint that import_item also exists in traitlets. The only difference between what you've added here and the version in traitlets is that the parameter is validated to be a string in the traitlets version. Since all of the uses of import_item in jupyter_client pass a string and jupyter_client has a dependency on traitlets - this seems like a good example of unnecessary duplication.

@Carreau
Copy link
Member Author

Carreau commented Mar 8, 2021

Good point, added a commit with the following:

Use import_item from traitlets,

As pointed out by Kevin, the method is already there in traitlets and
does more validation that the one in ipython_genutils.

I thought it was not in traitlets 4.x; but is indeed there and has been
for about 5 years.

And some import reshuffling to group similar imports from various packages.

I hear your concern about duplicated functionality; in particular for filefind; though I would argue that filefind is in this specific case wrong in particular because it still has some IPython specific behavior and expand specific values like IPYTHON_TEMP, and remove trailing/leading quotes because of magics. Unbundling it would allow to remove those behaviors; But I think that would be a candidate for another PR.

I can make utils private if that makes you more comfortable ?

@kevin-bates
Copy link
Member

Thanks for the update Matthias.

I would argue that filefind is in this specific case wrong in particular because it still has some IPython specific behavior and expand specific values like IPYTHON_TEMP, and remove trailing/leading quotes because of magics. Unbundling it would allow to remove those behaviors; But I think that would be a candidate for another PR.

I can make utils private if that makes you more comfortable ?

Since it's very likely that downstream applications will make use of the same functions in their upstream dependencies, and given these methods have some concerns, I'd be inclined to make these private until those concerns are addressed. We might also find that once the IPython-specific items have been addressed, they might make more sense in another package, etc. Thank you.

@Carreau
Copy link
Member Author

Carreau commented Mar 9, 2021

I've made the two functions in utils private.

@Carreau
Copy link
Member Author

Carreau commented Mar 9, 2021

Failures appear to be due to the new 6.0.0a0 of ipykernel.

@kevin-bates
Copy link
Member

Failures appear to be due to the new 6.0.0a0 of ipykernel.

I agree. In the meantime, should we adjust our CI dependency installation to not pull with --pre, or do you know if the issues understood and will be addressed soon?

IPython Genutils seem to start to be troublesome to package for some Linux
Ditributions because of Python2/3 compat. Get rid of it.

It was not in the dependencies anyway...
As pointed out by Kevin, the method is already there in traitlets and
does more validation that the one in ipython_genutils.

I thought it was not in traitlets 4.x; but is indeed there and has been
for about 5 years.
@Carreau
Copy link
Member Author

Carreau commented Mar 10, 2021

rebase on master to avoid the issues with --pre.

@Carreau Carreau added this to the 6.1.11 milestone Mar 10, 2021
@Carreau
Copy link
Member Author

Carreau commented Mar 10, 2021

Pushed update to the changelog to list most significant changes, and tagged many PRs with corresponding milestone for easy find.

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