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 with bound not working correctly #1053

Closed
albireox opened this issue Mar 15, 2021 · 18 comments
Closed

Generic with bound not working correctly #1053

albireox opened this issue Mar 15, 2021 · 18 comments
Labels
fixed in next version (main) A fix has been implemented and will appear in an upcoming version

Comments

@albireox
Copy link

albireox commented Mar 15, 2021

I'm having an issue with a generic that uses a TypeVar that needs to define an upper bound. The code is a bit convoluted, but here is the gist of it

Actor_co = TypeVar("Actor_co", bound=clu.base.BaseActor, covariant=True)

class Command(BaseCommand, Generic[Actor_co]):

    actor: Actor_co

    def __init__(
        self,
        command_string: str = "",
        transport: Optional[Any] = None,
        **kwargs,
    ):

        BaseCommand.__init__(self, **kwargs)
        ...

# AMQPActor is a subclass of clu.base.BaseActor
class ArchonActor(AMQPActor):
    ...

async def _do_one_controller(
    command: Command[archon.actor.ArchonActor],
    controller: ArchonController,
    exposure_params: dict[str, Any],
    exp_no: int,
    mjd_dir: pathlib.Path,
) -> bool:

    observatory = command.actor.observatory.lower()
    hemisphere = "n" if observatory == "apo" else "s"
    ...

When I define Command[archon.actor.ArchonActor] that produces an error

Type "ArchonActor" cannot be assigned to TypeVar "Actor_co@Command"
  Type "ArchonActor" is incompatible with bound type "BaseActor" for TypeVar "Actor_co@Command"
    "ArchonActor" is incompatible with "BaseActor"

but ArchonActor is a subclass of BaseActor. This example works well with mypy. If I remove the bound in the TypeVar the error disappears, but that produces some other issues with my code.

@erictraut
Copy link
Contributor

Could you simplify the sample so it's self-contained or give us more details on how to repro the problem? I can't tell from the above code where some of these symbols are imported or which package defines them.

I've tried filling in some of the missing type information below, and this type checks without a problem.

from typing import Any, Generic, Optional, TypeVar
import pathlib

class BaseActor:
    observatory: str

class AMQPActor(BaseActor): ...

class BaseCommand: ...

class ArchonController: ...

Actor_co = TypeVar("Actor_co", bound=BaseActor, covariant=True)


class Command(BaseCommand, Generic[Actor_co]):

    actor: Actor_co

    def __init__(
        self,
        command_string: str = "",
        transport: Optional[Any] = None,
        **kwargs,
    ):

        BaseCommand.__init__(self, **kwargs)
        ...


class ArchonActor(AMQPActor):
    ...

async def _do_one_controller(
    command: Command[ArchonActor],
    controller: ArchonController,
    exposure_params: dict[str, Any],
    exp_no: int,
    mjd_dir: pathlib.Path,
) -> bool:

    observatory = command.actor.observatory.lower()
    hemisphere = "n" if observatory == "apo" else "s"
    return True

@albireox
Copy link
Author

So I dug a bit more, and no, I cannot reproduce it with a single file. The problem seems to be related with using python.analysis.extraPaths. _do_one_controller and Command are defined in different libraries. Since I'm updating both of them at the same time, I had pointed the project for _do_one_controller to use the other library path with

"python.analysis.extraPaths": ["${workspaceFolder}/../clu/python"]

That was working in the sense that if I navigate to Command, I get the local version. But for some reason it's screwing with the type checking. If instead I install the library with

pip install -U ../clu/

and remove the extraPaths, then the error disappears. Not sure if I'm doing something wrong or there is a but in how extraPaths works.

@jakebailey
Copy link
Member

This may only moderately help, but if you're able to install the package and want to work on both, you may just want to use an editable install.

That extraPath should work in the same way, though (we do support workspaceFolder), so that's odd. Where does your selected interpreter live? Within the workspace you have open?

@albireox
Copy link
Author

Yes, I can use an editable install, but both projects use poetry so it's a bit more annoying because I have to edit the pyproject.toml and remember not to commit it.

The project I have open (the one that contains _do_one_controller) is installed in a virtual environment created with pyenv.

@albireox
Copy link
Author

It may be a problem with pyright. If I comment the extraPaths in the VSCode configuration and add a pyrightconfig.json with

{
  "executionEnvironments": [
    {
      "root": ".",
      "extraPaths": ["../clu/python"]
    }
  ]
}

I get the same behaviour (code navigation points to the correct version (../clu) but type checking seems to reference the version installed in site-packages. However, if I uninstall the site-packages version, then extraPaths seems to work.

@jakebailey
Copy link
Member

This might be microsoft/pyright#1515 then. I don't think extraPaths really works when used like this with relative paths in an execution environment.

@jakebailey
Copy link
Member

Hm, but extraPaths should still come before site-packages in the ordering, so I'm not sure the linked issue explains the discrepency, other than the overall "execution environments behave differently than one might think" situation; they're really only meant for self-contained folders that don't share code like this, IIRC.

@albireox
Copy link
Author

I tried with both relative and absolute paths, so I don't think relative paths is the issue. Code navigation worked correctly regardless of relative or absolute.

But yes, it may be that I'm using the execution environments (or the equivalent with the VSCode extraPaths) in a way that is not supported.

@jakebailey
Copy link
Member

Sorry, I really meant "pointing to code outside the execution environment itself"; we resolve relative paths before using them so it makes sense that pre-resolving them in your config doesn't change the behavior.

@jakebailey jakebailey added the needs investigation Could be an issue - needs investigation label Mar 15, 2021
@erictraut
Copy link
Contributor

An execution environment won't work because those "extraPaths" are applied only for files within that execution environment's root directory (and below). If you import a file outside of that directory, its imports will be resolved using its own execution environment settings — or the default settings if it falls outside of an execution environment. If it uses a non-relative import, it will probably be resolved using the selected Python environment. Pyright will interpret that as a separate module, so even if the types have the same local name, they won't be considered the same because they come from different source files.

Currently, pyrightconfig.json doesn't have a way of specifying extraPaths for the default execution environment (the one that's used when no other execution environments apply). We should probably add this, since it's already exposed via the "python.analysis.extraPaths" setting.

Let me make sure I understand your use case. It sounds like you are importing modules from a package that is checked out locally (outside of your workspace) but is not installed in any Python environment. We don't have good support for that currently. As Jake said, one workaround is to do an editable install of the package. That way, you can avoid "extraPaths" entirely, since Pylance will handle the import via your selected Python environment.

@github-actions github-actions bot removed the triage label Mar 15, 2021
@albireox
Copy link
Author

albireox commented Mar 15, 2021

I understand what you say. My current use-case is that I have two local repositories that I'm modifying at the same time (archon depends on clu so I keep expanding clu as more features are needed). In general, what I do is to use an editable install (as I say, that's slightly more complicated with poetry) or I override PYTHONPATH using Lua modules (this is less preferred because Pylance doesn't capture that, but it's useful for quick load/unload of the libraries).

In this case I was only modifying the typing in clu, so it seemed like a good case to use extraPaths since the code itself was not changing and I didn't want to modify the dependency specification, because that affects the GitHub Action tests.

Ultimately this is more a confusion than a real problem. Knowing that Pylance/pyright don't support that use-case, I can just remind myself to always use the editable install and make sure the interpreter is pointing to the version I want to lint.

What confuses me a bit is that the error is

Type "ArchonActor" cannot be assigned to TypeVar "Actor_co@Command"
  Type "ArchonActor" is incompatible with bound type "BaseActor" for TypeVar "Actor_co@Command"
    "ArchonActor" is incompatible with "BaseActor"

If Pylance was using the interpreter path to discover the clu library, I would have expected that the error would be something along the lines that Command doesn't accept a generic, since the version installed in the interpreter didn't have the specification of the generic.

@erictraut
Copy link
Contributor

I suspect you are seeing the error because "ArchonActor" is being imported from your local library (resolved via extraPaths), but "BaseActor" is being imported from the version of the package installed in your Python environment. From Pylance's perspective, the two types are defined by completely different source files and have no relation to each other.

@jakebailey, I think that "extraPaths" defined in the "python.analysis.extraPaths" directory should define the extraPaths for the default execution environment. Currently, we create a special execution environment with a root directory defined by the project root. That means any file outside of the project will not use these extraPaths for their imports. I think that's what's causing the confusion. If you agree with me here, I can make the changes to 1) expose extraPaths at the root level of pyrightconfig.json and 2) use this as a means to convey the "python.analysis.extraPaths" settings so it applies as a default.

@erictraut
Copy link
Contributor

@jakebailey, here's a PR for your review: microsoft/pyright#1632

@jakebailey
Copy link
Member

Thanks, I'll take a look. I'll want to verify that it won't regress when run as an LS, given we don't really have much testing for that situation.

@erictraut
Copy link
Contributor

@albireox, my change has been merged and will be included in the next release of pylance. This should eliminate the problem you're seeing when you use "python.analysis.extraPaths". The change also allows you to specify a top-level "default" extraPaths setting within pyrightconfig.json if you prefer to use that option. Thanks again for reporting the issue and providing us with additional details.

@erictraut erictraut added fixed in next version (main) A fix has been implemented and will appear in an upcoming version and removed needs investigation Could be an issue - needs investigation labels Mar 16, 2021
@albireox
Copy link
Author

Thanks for the super quick fix. I'll test it as soon as the new insiders pops up in my editor.

@jakebailey
Copy link
Member

Probably Wednesday; we don't really issue insiders other than the automatic one that comes after our stable build. Weekly releases are already pretty fast comparatively...

@jakebailey
Copy link
Member

This issue has been fixed in version 2021.3.2, which we've just released. You can find the changelog here: https://github.com/microsoft/pylance-release/blob/main/CHANGELOG.md#202132-17-march-2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed in next version (main) A fix has been implemented and will appear in an upcoming version
Projects
None yet
Development

No branches or pull requests

3 participants