Skip to content
Snippets Groups Projects

Find python package version via pkg_resources instead of pip-internal method

The pyversion.py script is called from DunePythonFindPackage.cmake and tries to find a package by its name, returning the extracted version number. Currently, it does so by checking the content of pip.get_installed_distributions().

However, in recent pip versions, pip.get_installed_distributions method was moved to pip._internal, leading to an AttributeError in the call to pyversion with packages that did not supply the __version__ variable.
Consequently, for those packages, DunePythonFindPackage will always evaluate to false.

What does this MR do?

This MR restores the behaviour of the pyversion.py script to look for the version of a package in other places then only the version variable.

It does so via pkg_resources.working_set, which offers access to the same information on installed distributions and is the recommended way. (Note, that it is strongly suggested to not use the pip._internal interface.)

Also, it removes the defunct implementation via the pip interface.

Open questions

Can it be assumed that pkg_resources is installed?
The get-pip.py docs suggest that it is installed unless explicitly suppressed by passing a flag, but I am not too familiar with the exact setup of the virtual environment...

Merge request reports

Pipeline #9522 passed

Approval is optional

Closed by Dominic KempfDominic Kempf 6 years ago (Jul 19, 2018 11:13am UTC)

Merge details

  • The changes were not merged into master.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Currently, the build seems to time out on the Debian:10 gcc:c++17 job. Seems unrelated to the changes implemented here, though?

    /builds/yunus.sevinchan/dune-common/dune/common/simd/test.hh: In static member function 'static T Dune::Simd::UnitTest::prvalue(T) [with T = Vc_1::SimdArray<double, 8>]':
    /builds/yunus.sevinchan/dune-common/dune/common/simd/test.hh:142:16: note: The ABI for passing parameters with 64-byte alignment has changed in GCC 4.6
           static T prvalue(T t)
  • The note is unrelated to the timeout. I actually prepared a patch for the CI system in joe/dune-docker!1 to make it possible to skip expensive tests, but apparently, I did not prod @ansgar properly.

  • Thanks for looking into the failing job!

    The note is unrelated to the timeout.

    My bad, sorry.

    The retries of the job seem to have run through, though. Is there anything I can do right now? Or anyone I can/should tag to have a look over this?

  • Nothing you need to do at the moment.

    If you change the MR you might hit the same problem again though. In that case the best thing you can do is hit retry. If that does not succeed after a few tries, prod me again. (Or prod Dominic/Steffen directly, if my guess is right and you're from Heidelberg.)

    Just to make this clear: I'm not going to merge this because I feel unqualified to judge this. I'm assuming you're in contact with someone else about merging.

  • If you change the MR you might hit the same problem again though. In that case the best thing you can do is hit retry. If that does not succeed after a few tries, prod me again.

    Alright, thanks! :)

    (Or prod Dominic/Steffen directly, if my guess is right and you're from Heidelberg.)

    It is. Updated my profile now so that that's clear.

    I'm assuming you're in contact with someone else about merging.

    Not yet, but will do.

  • @dominic: Would you be the right person to prod here?

    I'm working in the Roth Group at the IUP (together with Lukas Riedel) and we came to use DUNE more extensively the last months. The issue that this MR aims to solve originated from that work.
    Sorry for not introducing myself earlier or getting in touch with anyone! I thought I'll just propose this MR and see what happens. :)

  • Dear Yunus, I will look into this next week.

  • assigned to @dominic

  • So, I had a look and also stumbled over the same github issue. I think we should follow their strong hint of not using stuff from pip._internal. I think it is safe to assume pkg_resources presence here - you get it alongside setuptools, which should be present. If it breaks, we can still formulate it as a hard requirement. Otherwise, this is a valid concern and I will merge it once you have updated it.

  • Thanks for looking into it!

    I will merge it once you have updated it.

    I'm not fully clear on this: which updates would you wish to have incorporated? The hard requirement of pkg_resources?

  • I meant reimplementation with the method proposed in this post

  • Ah, ok. That's already done here. :)

    I'm only using a dict instead of a list; that way, we have the name of the packages as keys and can more easily check for the presence of a certain package:

    # Generate a dict of distribution information with project names as keys
    dist_info = {d.project_name: d for d in pkg_resources.working_set}
    
    # Check if package is available at all
    if modstr in dist_info:
        # ... can now check for the version
  • The issue of the runner timing out should be mitigated (!502 (merged), merged into master). Although this MR won't benefit from that, unless you merge master into it, or rebase it on top of master.

    Edited by Jö Fahlke
  • Yunus Sevinchan added 37 commits

    added 37 commits

    • e07f397e...ec9401e6 - 36 commits from branch core:master
    • 467c9420 - Merge branch 'master' into feature/do-not-use-pip-internals-for-pyversion

    Compare with previous version

  • I meant reimplementation with the method proposed in this post

    @dominic So would you insist on that specific implementation? Or is there something else wrong with the current state of this MR? Just let me know if there is anything else to do :)

  • Yunus, I am sorry for my handling of this merge request. I originally misread something (lack of sleep/concentration while at conference) and then I went on leave for a month without looking into this. In the mean time @smuething discovered and fixed the same bug in !540 (merged), but not this merge request. Long story short, it should work on master and 2.6 now.

  • closed

  • Thanks for the clarification, Dominic. No worries. Good that it's fixed now. :)

Please register or sign in to reply
Loading