Setup suggestions for VSCode

scottanderson42 commented 1 year ago

Documentation issue

[ ] Reporting a typo
[ ] Reporting a documentation bug
[ x ] Documentation improvement
[ ] Documentation feedback

Is there a specific documentation page you are reporting?

We're looking for suggestions on setting up VSCode with the pyproject.toml-per-project approach. Since there isn't a single .venv for the workspace Pylance doesn't seem to quite know what to do with the individual libraries and their imports, as you can see here:

Pylance can't find the import, and there are no linting errors for the types on the functions.

lucasvieirasilva commented 1 year ago

Hey @scottanderson42, I can see 3 options.

Using a shared virtual environment (Check this article https://betterprogramming.pub/poetry-python-nx-monorepo-5750d8627024 in the Shared Virtual Environment section).
Using VSCode Workspaces, each project has the .vscode pointing to the correct virtual environment.
Switching virtual environment in the VSCode Select Interpreter Menu

I usually use the Shared virtual environment also to avoid creating tons of virtual environments in my CI/CD pipeline, but let me know if that doesn't work for you, so, we can think about a better way to switch the virtual environment in a simpler way.

scottanderson42 commented 1 year ago

Well, that's the thing: we're starting with separate virtual environments, one per project, to avoid problems with incompatibility when using libraries like numpy and pytorch. Some of the AI models we use can have very particular version requirements and they don't play well together.

lucasvieirasilva commented 1 year ago

makes sense, and depending on the number of projects you have, switching the virtual environment in the VSCOde select interpreter might be not so productive, let me do some research about it, and see if there are some options to activate the virtual environment based on the folder, I'll let you know, sounds good?

scottanderson42 commented 1 year ago

Sounds great, appreciate it!

lucasvieirasilva commented 1 year ago

cool, in the meantime you can switch the virtual environment using the select interpreter menu, Pylance will stop complaining about references.

lucasvieirasilva commented 1 year ago

Hey @scottanderson42, after some research, I didn't find a way to make VSCode identify the virtual environment by folder, someone posted a question on the python VSCode plugin repo https://github.com/microsoft/vscode-python/issues/9673 and we can't change the interpreter programmatically, well, we can change the settings.json, but this requires a custom VSCode plugin and could be confusing git wise changing the same file depending on your usage, I don't think it a good approach.

However, I did some tests using VSCode workspaces using the same example workspaces we were using yesterday, and in this way VSCode can easily switch the virtual environment based on the project.

there is a discussion opened https://github.com/microsoft/vscode-python/issues/21204, that could solve permanently this issue in the future.

workspace.code-workspace

{
    "folders": [
        {
            "name": "root",
            "path": "."
        },
        {
            "name": "apps / test-app",
            "path": "apps/test-app"
        },
        {
            "name": "libs / test-one",
            "path": "libs/test-one"
        },
        {
            "name": "libs / test-two",
            "path": "libs/test-two"
        }
    ],
    "settings": {}
}

And for each Python project:

.vscode/settings.json (apps/test-app/.vscode/settings.json)

{
    "python.defaultInterpreterPath": "./.venv/bin/python",
}

In this way, VSCode can use the correct virtual environment,

What do you think?

Note: I can handle those files inside the @nxlv/python generator so you don't need to update those files when you add a new project.

scottanderson42 commented 1 year ago

I tried it out, thanks for working on this.

Linting works! ✨ Or at least it did until it stopped working, not sure what changed.

Testing in the terminal does not work: npx nx run test-app:test ->

from test_app import app
test_app/app.py:1: in <module>
from test_one.index import hello
E   ModuleNotFoundError: No module named 'test_one'

I'm not sure if tracking the .code-workspace file is kosher or not. Most of my VSCode users are out this week so I'll take that up in a discussion with the group when they're back.

If you do go this route, I'd volunteer to write a plugin to manage .dir-locals.el files for Emacs, which accomplish the same thing as the nested settings.json files do for VSCode.

lucasvieirasilva commented 1 year ago

Hey @scottanderson42 regarding the testing command, can you check that your terminal doesn't have any Python virtual environment activated?

Because sometimes when you select the Python interpreter in the VSCode and open a terminal it activates in the terminal automatically, and if it is a different virtual environment then test-app is not gonna work.

Regarding the .dir-locals.el files for Emacs, interesting idea, I've never done it before.

We can discuss more about it, I'm happy to help!

scottanderson42 commented 1 year ago

The wrong .venv being activated was the issue. I can fix this by changing the command for the "test" task to:

"command": ". .venv/bin/activate ; poetry run pytest tests/"

This won't work for shared .venv setups though.

scottanderson42 commented 1 year ago

Idea: Hybrid Project Structure

Random idea I had on a walk this morning: for repos with individual .venvs, also maintain the central .venv at the root of the workspace. I'm not sure this is better than fighting the local .vscode/settings.json configuration but I wanted to put it in front of you for your thoughts.

Benefits over just individual .venvs:

single place to install dev dependencies in the root
eases the configuration in editors for linting, Intellisense, and so on
works better with VSCode's currently cranky multi-root implementation
VSCode needs the linters to be installed in whatever .venv is active, apparently. So without a root .venv or a global linter installation, it can't find the executables.

Benefits over just a single .venv:

for app deployments, the local pyproject.toml keeps the package size down because it's only including the necessary libraries
current state-of-the-art for Python tree-shaking doesn't appear to be good, so using a single venv could grow beyond the size limits of cloud deployments (eg. AWS limits Lambda packages to 50M zipped, 250M unzipped)
a single root venv helps keep all of the individual venvs using the same versions of packages

Cons:

Double installations of Python packages! However, using a single cache directory for all of the venvs in the workspace will mitigate this and the speed shouldn't suffer much.
More complicated and easy to break if developers forget to use the plugin tasks
Some linters, like Black, look for local pyproject.toml files, so Black configuration would either need to be duplicated in each project, or always given the root configuration file as a command line argument.

Alternative to the above idea: "tracked tree-shaking"

In this idea, don't create the individual .venvs, but keep pyproject.toml up to date so it can be used to generate requirements.txt for deploys. This approach gets rid of the duplication of Python packages.

lucasvieirasilva commented 1 year ago

Hey @scottanderson42, thanks for thinking about it, well, I use this approach in my projects, of course, there are some limitations related to version conflicts, but it is the best approach in my point of view because you can use it easily in VSCode, it is also faster to install in the CI/CD pipeline.

There is an Nx generator to convert the individual .venv to a shared one:

npx nx generate @nxlv/python:migrate-to-shared-venv

As you described, there will be a pyproject.toml in the root that references the projects as local dependencies and keep the poetry.lock up to date in all the projects and also the root one.

@nxlv/python detects if you have a pyproject.toml in the root and automatically updates the poetry.lock when you add a new dependency in one of the projects.

Example.

npx nx run test-app:add --name pendulum

This command is gonna add the pendulum as a dependency of test-app but also updates the poetry.lock from the root.

Could you play a little bit with this feature and let me know if that fits your use case?

Have a nice weekend

scottanderson42 commented 1 year ago

For some reason I thought the shared root venv removed the pyproject.toml files for all of the subprojects. Is that not the case then?

lucasvieirasilva commented 1 year ago

Hey @scottanderson42, no, it's not, it only maintains a pyproject in the root and keeps all in sync, all the projects have their own pyproject and they can have different packages as dependencies

The only limitation is the versions needs to be the same.

scottanderson42 commented 1 year ago

Hi @lucasvieirasilva, my apologies for being confused about how the root venv configuration worked. Figures you were already ahead of me there. 😄

I gave it a try by converting my test workspace. The conversion went fine, the root .venv is there and activated, but the tests are failing.

$ which python                                                                                                                                                           1 ↵
/Users/anderson/src/tmpnx15/.venv/bin/python
$ npx nx run test-app:test

> nx run test-app:test

============================= test session starts ==============================
platform darwin -- Python 3.8.12, pytest-7.1.2, pluggy-1.0.0
rootdir: /Users/anderson/src/tmpnx15/apps/test-app, configfile: pyproject.toml
plugins: Faker-11.3.0, requests-mock-1.8.0
collected 0 items / 1 error
==================================== ERRORS ====================================
_____________________ ERROR collecting tests/test_index.py _____________________
ImportError while importing test module '/Users/anderson/src/tmpnx15/apps/test-app/tests/test_index.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
../../../../.pyenv/versions/3.8.12/lib/python3.8/importlib/__init__.py:127: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/test_index.py:1: in <module>
    from test_app import app
test_app/app.py:1: in <module>
    from test_one.index import hello
E   ModuleNotFoundError: No module named 'test_one'

lucasvieirasilva commented 1 year ago

Hey @scottanderson42, no worries, did you activate the root venv in your terminal before running the command?

scottanderson42 commented 1 year ago

Yes, whoops, I formatted the code wrong. As you can see now though the python is from the root .venv.

lucasvieirasilva commented 1 year ago

Interesting, can I see your root pyproject?

scottanderson42 commented 1 year ago

[tool.poetry]
name = "tmpnx15"
version = "1.0.0"
description = ""
authors = [ ]
license = "Proprietary"
readme = "README.md"

  [tool.poetry.dependencies]
  python = "^3.8.12"

    [tool.poetry.dependencies.test-app]
    path = "apps/test-app"
    develop = true

    [tool.poetry.dependencies.test-one]
    path = "libs/test-one"
    develop = true

    [tool.poetry.dependencies.test-two]
    path = "libs/test-two"
    develop = true

[build-system]
requires = [ "poetry-core==1.1.0" ]
build-backend = "poetry.core.masonry.api"

lucasvieirasilva commented 1 year ago

Hmm, looks correct, can check if your root venv has the test_app in the lib folder?

scottanderson42 commented 1 year ago

Sure.

$ ls -l .venv/lib/python3.8/site-packages
total 128
drwxr-xr-x   3 anderson  staff     96 Jun  3 17:08 __pycache__
drwxr-xr-x   5 anderson  staff    160 Jun  3 17:08 _distutils_hack
-rw-r--r--   1 anderson  staff     18 Jun  3 17:08 _virtualenv.pth
-rw-r--r--   1 anderson  staff   4311 Jun  3 17:08 _virtualenv.py
drwxr-xr-x  13 anderson  staff    416 Jun  3 17:19 dateutil
-rw-r--r--   1 anderson  staff    151 Jun  3 17:08 distutils-precedence.pth
drwxr-xr-x  21 anderson  staff    672 Jun  3 17:19 pendulum
drwxr-xr-x   7 anderson  staff    224 Jun  3 17:19 pendulum-2.1.2.dist-info
drwxr-xr-x   7 anderson  staff    224 Jun  3 17:08 pip
drwxr-xr-x   9 anderson  staff    288 Jun  3 17:08 pip-22.0.2.dist-info
-rw-r--r--   1 anderson  staff      0 Jun  3 17:08 pip-22.0.2.virtualenv
drwxr-xr-x   6 anderson  staff    192 Jun  3 17:11 pkg_resources
drwxr-xr-x   9 anderson  staff    288 Jun  3 17:19 python_dateutil-2.8.2.dist-info
drwxr-xr-x   9 anderson  staff    288 Jun  3 17:19 pytzdata
drwxr-xr-x   7 anderson  staff    224 Jun  3 17:19 pytzdata-2020.1.dist-info
drwxr-xr-x  42 anderson  staff   1344 Jun  3 17:11 setuptools
drwxr-xr-x   9 anderson  staff    288 Jun  3 17:08 setuptools-60.6.0.dist-info
-rw-r--r--   1 anderson  staff      0 Jun  3 17:08 setuptools-60.6.0.virtualenv
drwxr-xr-x   8 anderson  staff    256 Jun  3 17:19 six-1.16.0.dist-info
-rw-r--r--   1 anderson  staff  34549 Jun  3 17:19 six.py
drwxr-xr-x   6 anderson  staff    192 Jun  3 17:11 test_app-1.0.0.dist-info
-rw-r--r--   1 anderson  staff     42 Jun  3 17:11 test_app.pth
drwxr-xr-x   7 anderson  staff    224 Jun  3 17:11 test_one-1.0.0.dist-info
-rw-r--r--   1 anderson  staff     42 Jun  3 17:11 test_one.pth
drwxr-xr-x   7 anderson  staff    224 Jun  3 17:11 test_two-1.0.0.dist-info
-rw-r--r--   1 anderson  staff     42 Jun  3 17:11 test_two.pth
drwxr-xr-x  12 anderson  staff    384 Jun  3 17:08 wheel
drwxr-xr-x   9 anderson  staff    288 Jun  3 17:08 wheel-0.37.1.dist-info
-rw-r--r--   1 anderson  staff      0 Jun  3 17:08 wheel-0.37.1.virtualenv

lucasvieirasilva commented 1 year ago

Well, it looks right, all the local dependencies are there, should work, did you remove the shell code that you added to activate the venv before running the test in the test target inside the project.json?

scottanderson42 commented 1 year ago

Yes, I did. Everything looks like it should work.

lucasvieirasilva commented 1 year ago

Yes, the venv is ok, activated in the terminal, the target command is also ok, I don't know hehe, I need to reproduce that, sorry man, I'm not at home now, I get back Monday, I feel that is something very silly

scottanderson42 commented 1 year ago

No worries, it's not a blocker and we'll figure it out. Enjoy your weekend.

lucasvieirasilva commented 1 year ago

Thanks man, you too!

lucasvieirasilva commented 1 year ago

Hey @scottanderson42 I could reproduce the issue here, it is something really silly.

In the @nxlv/python project generator when you create a new project, the template contains some pytest addons configuration and runs the unit test using pytest command line.

However, the pytest dependency was never installed in the workspace, it is working when we have the individual venv in the project level because the test_one module dependency is referenced there.

But when we move to the root virtual environment pytest (from external sources) could not identity as a virtual environment, even with the virtual environment activated.

I output the sys.path from pytest and run python command in the terminal inside the project folder, look at the difference:

npx nx test test-app (root venv activated)

['/Users/lucasvieira/Projects/issues/nx-plugins/issue-sam/packages/test-app', '/usr/local/bin', '/usr/local/Cellar/python@3.9/3.9.16/Frameworks/Python.framework/Versions/3.9/lib/python39.zip', '/usr/local/Cellar/python@3.9/3.9.16/Frameworks/Python.framework/Versions/3.9/lib/python3.9', '/usr/local/Cellar/python@3.9/3.9.16/Frameworks/Python.framework/Versions/3.9/lib/python3.9/lib-dynload', '/Users/lucasvieira/Library/Python/3.9/lib/python/site-packages', '/usr/local/lib/python3.9/site-packages']

It only has the test-app path and all my global python paths.

Command: cd packages/test-app; python

Code:

import sys
print(sys.path)

Output:

['', '/Users/lucasvieira/.pyenv/versions/3.9.5/lib/python39.zip', '/Users/lucasvieira/.pyenv/versions/3.9.5/lib/python3.9', '/Users/lucasvieira/.pyenv/versions/3.9.5/lib/python3.9/lib-dynload', '/Users/lucasvieira/Projects/issues/nx-plugins/issue-sam/.venv/lib/python3.9/site-packages', '/Users/lucasvieira/Projects/issues/nx-plugins/issue-sam/packages/app1', '/Users/lucasvieira/Projects/issues/nx-plugins/issue-sam/packages/lib1', '/Users/lucasvieira/Projects/issues/nx-plugins/issue-sam/packages/test-app', '/Users/lucasvieira/Projects/issues/nx-plugins/issue-sam/packages/test-one', '/Users/lucasvieira/Projects/issues/nx-plugins/issue-sam/packages/test-two']

You can see that all local paths are present,

So, I thought that needs to be something related to the pytest reference.

Then, I installed the pytest dependencies in the root level

poetry add pytest pytest-cov pytest-html

Then everything works fine.

Could you try adding those pytest pytest-cov pytest-html packages and see if works for you too?

If yes, I guess I need to change the npx nx generate @nxlv/python:migrate-to-shared-venv generator to add pytest dependencies to avoid this issue.

scottanderson42 commented 1 year ago

That fixes it, yes! Thanks for the sleuthing. Does it make sense to instead copy all of the dev dependencies from the individual projects, as opposed to hardcoding pytest?

lucasvieirasilva commented 1 year ago

Hey @scottanderson42, yes it does, and I guess the changes are also related to the issues you opened last week #111,

Currently, by default, the project generator does not add the pytest dependencies, so, if the developer runs the command to migrate to the shared virtual environment, it will not migrate correctly, as you saw when you ran and it didn't work.

lucasvieirasilva / nx-plugins