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

Generic Python package discovery #824

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Generic Python package discovery #824

wants to merge 7 commits into from

Conversation

gnikit
Copy link
Member

@gnikit gnikit commented Feb 5, 2023

This PR should add support to allow for the discovery of:

  • packages installed in virtual environments
  • packages installed in Python environments that are not in the PATH
  • Python environments setup as default ms-python.python

TODO:

  • Update description
  • Assign Issues
  • Add tests

@codecov
Copy link

codecov bot commented Feb 5, 2023

Codecov Report

Merging #824 (ff98bed) into main (427e8df) will decrease coverage by 12.75%.
The diff coverage is 23.36%.

❗ Current head ff98bed differs from pull request most recent head 2029f36. Consider uploading reports for the commit 2029f36 to get more accurate results

@@             Coverage Diff             @@
##             main     #824       +/-   ##
===========================================
- Coverage   83.64%   70.89%   -12.75%     
===========================================
  Files          12       16        +4     
  Lines        2311     2783      +472     
  Branches      205      208        +3     
===========================================
+ Hits         1933     1973       +40     
- Misses        373      805      +432     
  Partials        5        5               
Files Coverage Δ
src/format/provider.ts 69.69% <100.00%> (ø)
src/lint/provider.ts 81.70% <100.00%> (-0.18%) ⬇️
src/lsp/client.ts 66.58% <100.00%> (-0.25%) ⬇️
src/util/tools.ts 74.20% <100.00%> (-5.92%) ⬇️
src/util/shell.ts 98.63% <98.63%> (ø)
src/util/python.ts 68.29% <68.29%> (ø)
src/util/ms-python-api/jupyter/types.ts 0.00% <0.00%> (ø)
src/util/ms-python-api/types.ts 0.00% <0.00%> (ø)

@gnikit gnikit mentioned this pull request Aug 4, 2023
1 task
@gnikit gnikit changed the title ci: enable windows testing Generic Python package discovery Aug 4, 2023
@gnikit gnikit self-assigned this Aug 4, 2023
@michaelkonecny
Copy link

Any way I can help with this?

@gnikit
Copy link
Member Author

gnikit commented Aug 7, 2023

I have a more advanced version of this branch staged locally, that wraps all existing Python functionality and some additional features into a Python class. However the challenges I am facing are the following.

I can't seem to be able to find a good SOLID approach to share a single instance of the Python class with all the different parts of the extension. Specifically the Single Responsibility Principle appears to be violated repeatedly with my proposed design.
There is a package called inversify (https://github.com/inversify/InversifyJS) that could help with the dependency inversion and SRP, but I would have to tear apart most of the extension. Using inversify would only make sense if I had the time to rewrite certain parts of the extension that deal with the whole set of Configuration options. Inverting the dependency just for Python but maintaining the linter and formatting dependencies is somewhat strange.

The second part is that all of this Python work feels somewhat redundant given that we could save ourselves a lot of grief just by using pipx run fortls or pipx run findent. My main concerns with pipx are that:

  • it spawns considerably slower than native calls to fortls (or any other binary)
  • It uses caching to improve performance but the cache expires after 14 days
  • It does not respect virtual environments, so pipx run fortls will install fortls in whichever place pipx installs binaries, even if pipx is called from within a virtual environment. I am sure there might be some settings that can help with this, but I haven't found them.

I would be fine using pipx if it weren't for the last point. I suspect users might complain that Modern Fortran installs stuff outside their venv.

As you can tell, my mind is not entirely set with the design or even approach that should be followed to solve the Python issues.

Let me know what you think.

@michaelkonecny
Copy link

michaelkonecny commented Aug 10, 2023

Inverting the dependency just for Python but maintaining the linter and formatting dependencies is somewhat strange.

Yeah that seems strange to me as well.

I'm not sure I quite understand the motivation for this PR.
From my perspective, there's the problem of discovering fortls on Windows, because it typically gets installed in a folder that's not on PATH, but is this the case on linux/mac as well?
Can you clarify?

The way I look at it now is there are basically two use cases:

  1. we want to make the usage of the extension as seamless as possible,
    which means being able to install, update, detect and run fortls automatically
  2. the users should have the option to override this and run fortls from a custom location

Is this correct?

For the user override, do we have any idea on how often people actually use it?
If it were just a very small number, it might not be worth maintaining in the long term if it complicates things too much, but I have no idea how popular it is or how it's used.

@gnikit
Copy link
Member Author

gnikit commented Aug 10, 2023

I'm not sure I quite understand the motivation for this PR.
From my perspective, there's the problem of discovering fortls on Windows, because it typically gets installed in a folder that's not on PATH, but is this the case on linux/mac as well?
Can you clarify?

The motivation is to be able to interact with Python in a standardised and abstract manner.
Discovery of fortls on Windows is just one of the issues that would be resolved.
There can be similar issues with formatters, fypp preprocessor and fpm.
There is an entire class of user setups that would face auto-discovery issues.
Similarly, there are virtual environments and interactions with the ms-python.python extension that have been requested as features.

Manually specifying the paths, although well documented and trivial to do, seems to be a blocker for many users.
To me that is a clear indicator that the extension design of how Python package auto-discovery is implemented needs to be reworked.

Modern Fortran, as an extension that integrates with Python packages needs to:

  1. Work with multiple Python and virtual environments
  2. Install packages to the selected Python environment
  3. Check if a Python package is installed in the environment
  4. Get the location of installed Python binaries in the environment
  5. Share the selected Python configuration within all parts of the extension that make use of it

This PR aims to address all of the above bullet points.
There are some minor issues, mostly related to points 3. & 4., but nothing that can't be solved with a bit of Python code.
The more pressing issues are 5. & 1. which carry technical difficulties and design considerations.

@michaelkonecny
Copy link

I see.

Regarding the point:

  1. Work with multiple Python and virtual environments

Not with multiple ones at a time, though, right?
What's the idea about how one would select the venv that's supposed to be used?

If your local code isn't too much of a mess, maybe you can push it to a separate branch and I could have a look at it? Maybe I'll have some ideas. (It doesn't have to work, just to give me an idea...)

@dnwillia-work
Copy link

dnwillia-work commented Sep 20, 2023

I have a more advanced version of this branch staged locally, that wraps all existing Python functionality and some additional features into a Python class. However the challenges I am facing are the following.

I can't seem to be able to find a good SOLID approach to share a single instance of the Python class with all the different parts of the extension. Specifically the Single Responsibility Principle appears to be violated repeatedly with my proposed design. There is a package called inversify (https://github.com/inversify/InversifyJS) that could help with the dependency inversion and SRP, but I would have to tear apart most of the extension. Using inversify would only make sense if I had the time to rewrite certain parts of the extension that deal with the whole set of Configuration options. Inverting the dependency just for Python but maintaining the linter and formatting dependencies is somewhat strange.

The second part is that all of this Python work feels somewhat redundant given that we could save ourselves a lot of grief just by using pipx run fortls or pipx run findent. My main concerns with pipx are that:

  • it spawns considerably slower than native calls to fortls (or any other binary)
  • It uses caching to improve performance but the cache expires after 14 days
  • It does not respect virtual environments, so pipx run fortls will install fortls in whichever place pipx installs binaries, even if pipx is called from within a virtual environment. I am sure there might be some settings that can help with this, but I haven't found them.

I would be fine using pipx if it weren't for the last point. I suspect users might complain that Modern Fortran installs stuff outside their venv.

As you can tell, my mind is not entirely set with the design or even approach that should be followed to solve the Python issues.

Let me know what you think.

Honestly, I think you could probably abandon this PR. Virtual environments seem to work totally fine as far as I can tell and this is the standard way to setup non-default Python environments. See my comment in #957

@gnikit gnikit marked this pull request as draft November 6, 2023 22:57
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.

3 participants