Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

How can I disable the bundled SQLAlchemy stubs? #837

Closed
archfear opened this issue Jan 15, 2021 · 18 comments
Closed

How can I disable the bundled SQLAlchemy stubs? #837

archfear opened this issue Jan 15, 2021 · 18 comments
Labels
enhancement New feature or request reference

Comments

@archfear
Copy link

My code is incompatible with the SQLAlchemy stubs bundled in the latest version of Pylance. Is there a way to disable these via the config file or adding something to the typings directory?

Are these stubs identical to the latest release of https://github.com/dropbox/sqlalchemy-stubs or do they include functionality similar to the mypy plugin included in that repo?

@jakebailey
Copy link
Member

jakebailey commented Jan 15, 2021

The stubs are identical to that repo, but without the mypy plugin (as we aren't mypy and can't use it).

We don't have a way to disable them, no, as we assumed they'd be better than trying to read the actual source code. As a workaround, you can just delete the folder from the extension folder itself and reload the editor.

Can you describe the incompatibility? Are you referring to type checking, or something else? How was your code passing beforehand when SQLAlchemy isn't typed?

@smithed180
Copy link

smithed180 commented Jan 18, 2021

I may just not be doing this right, but I can explain the behavior I see. Prior to the latest release of pylance, I had manually set all of the types for all of the column fields. For example, I might have:

class blah(base):
    id:int = Column(Integer)

I know this is technically wrong, but it make it so consumers of the code could see the right types when accessing the class. As of the latest pylance release, this generates the big red squiggles with an error that says Column[int] is not compatible with int.

Unfortunately I also have some classes where I never added the type hinting because they were more-or-less self contained:

class runRecord(base):
    username=Column(Text)
    runTime=Column(DateTime)
    def __init__(self):
        self.runTime = datetime.now()

When another part of the code tries to write a string to the username field, I get:

Cannot assign member "username" for type "runRecord"
  Expression of type "str" cannot be assigned to member "username" of class "runRecord"
    "str" is incompatible with "Column[Text]"

Small update: if you assign a value in the constructor, the type hint system appears to use that rather than the Column type. In other words if in runRecord.init I had self.username = '' the type hinter doesn't generate squiggles. This makes perfect sense, but it seemed worth mentioning.

Is there a 'right' way to annotate these?

@judej judej added the waiting for user response Requires more information from user label Jan 19, 2021
@github-actions github-actions bot removed the triage label Jan 19, 2021
@jakebailey
Copy link
Member

jakebailey commented Jan 19, 2021

@archfear I'd still like to get some info about what's going on in your codebase. I'm not entirely certain it's the stubs, but potentially another change (maybe the same as below). Deleting the stubs would be a useful datapoint.

@smithed180 What you're seeing is likely an impact of #822; in the "off" type checking mode, there was a special case where we'd "allow" the bad type assignment in an effort to be more friendly. But, I'm a bit confused since in the "off" mode you shouldn't see that error message at all. Do you have type checking enabled?

If you wanted to annotate that and force the type to be int, I'd think you'd need a # type: ignore comment on that line to suppress the message, since it is giving a valid message by sating "you can't assign Column[int] to int as they aren't compatible. SQLAlchemy is unfortunate from the POV that you do a bunch of assignments to build up a class, but the code you write don't actually reflect the types that are there at runtime...

@smithed180
Copy link

smithed180 commented Jan 19, 2021

Thanks Jake -- it sounds like my original iffy way of handling things is still best. Pylance is still a ton better at inferring things...its nice when you argue with the tool about types and then find out that it was right and you had a bug, so dealing with sqlalchemy's weirdness is a small price to pay.

As for the 'off' mode, is it correct to assume you mean this setting? If so I've always had it set this way:
"python.analysis.typeCheckingMode": "basic"
I wasn't 100% what the options meant when I installed (off is described as having no type checking, while basic is "all rules in off + basic"...but I thought off had no rules....) So anyway I just picked basic because it sounded like "medium" and "medium" seemed like a fair starting point :)

Edit: is there a way to turn off type checking in either of these two ways:

  • An entire block. I have this class with N columns, I want to disable type checking for the block
  • Just a portion of a line. For example, if I have blah:int=column(Integer, thing1=True, thing2=1.23) I want the type checker to still check that thing1 and thing2 are the right types, just want it to ignore the overall assignment and let me claim that blah is an int.

sorry for hijacking a little bit

@jakebailey
Copy link
Member

An entire block. I have this class with N columns, I want to disable type checking for the block

There isn't a way to do this. #282

Just a portion of a line. For example, if I have blah:int=column(Integer, thing1=True, thing2=1.23) I want the type checker to still check that thing1 and thing2 are the right types, just want it to ignore the overall assignment and let me claim that blah is an int.

You might be able to split it into many lines and then stick # type: ignore on just one of them, but that may not work either. For this case, the assignment is the problem, so as gross as it sounds, you could also do blah = cast(int, column(...)), but I know that feels wrong too.

@archfear
Copy link
Author

archfear commented Jan 19, 2021

@jakebailey My situation is similar to @smithed180. I was having issues getting Dropbox's SQLAlchemy stubs to work with Flask-SQLAlchemy and my app's architecture. I annotated the columns in the same way as @smithed180 to at least get type checking on the attributes of the model instance. This works fine in strict mode as long as the SQLAlchemy stubs aren't installed. I needed to add # type: ignore when calling a function on a model attribute such as in_ from within a filter() call and annotate the results of a query since they'll be untyped.

I'll be removing Flask-SQLAlchemy from my app in the future and plan to revisit getting the stubs working then.

@jakebailey
Copy link
Member

I see, so previously SQLAlchemy was untyped, and now that it's typed you're getting mismatches? I'm curious what the inferencing was doing without the stubs, then, as if you were in strict mode I would have expected some errors for using types that were inferred. We had assumed that adding these stubs would only improve things.

Just to be clear, if you delete the sqlalchemy folder from the pylance extension, do things work as they previously did, or are there still errors?

@smithed180
Copy link

smithed180 commented Jan 19, 2021

You might be able to split it into many lines and then stick # type: ignore on just one of them, but that may not work either. For this case, the assignment is the problem, so as gross as it sounds, you could also do blah = cast(int, column(...)), but I know that feels wrong too.

Ah yeah should have just tried the multiline thing -- can confirm it does not work. The "1" here should (and does, without the ignore comment) cause a type error:

    id:int = Column( #type:ignore
        1, Integer)

This one still generates the error on the first line

    id = Column('id', Integer, primary_key=True, autoincrement=True, nullable=False)
    id: int #type:ignore

Most of the sqlalchemy things are keywords anyway so I'll just do the type ignore. Thanks for the assistance.

@archfear
Copy link
Author

@jakebailey Removing the types from dist/bundled/stubs or downgrading the extension both fix the issue. I should be more clear about my config: typeCheckingMode is set to strict, but useLibraryCodeForTypes is false and I've disabled reportMissingTypeStubs and all the reportUntyped* and reportUnknown* rules.

Without the stubs, everything referenced from SQLAlchemy is inferred as Unknown other than the model attributes I explicitly annotated.

One possible workaround here would be to allow the stubs in a project's typings directory take precedence over the bundled stubs. This would allow for using a specific version of the SQLAlchemy stubs in a project or disabling them using the technique described here: microsoft/pyright#945 (comment)

@jakebailey
Copy link
Member

The typings directory should definitely take precedence over the bundled stubs; the intended order is typings, then installed (search paths), then bundled as a last resort. Are you seeing a different behavior?

@archfear
Copy link
Author

@jakebailey I can confirm that a copy of the stubs in typings takes precedence over the bundled stubs. I didn't attempt to disable them by putting an empty stub in typings.

After a lot of trial and error, I managed to get the sqlalchemy stubs working reasonably well with Pyright and Pylance. Since there's practically no information out there on how to do this, I'll share what I've found.

Flask-SQLAlchemy

If you're using Flask along with Flask-SQLAlchemy, you can't use the query attribute on the models.

use this:

db.session(User).get(id)

not this:

User.query.get(id)

Models

When defining models, columns must be explicitly cast to their associated Python types. When defining relationships, both sides of the relationship should be defined separately using back_populates as opposed to defining both sides on one model using backref.

class User(Base):
    id = cast(
        str,
        Column(
            "id", UUID(), primary_key=True, server_default=text("uuid_generate_v4()")
        ),
    )
    organization_id = cast(
        str, Column(UUID(), ForeignKey("organization.id"), index=True, nullable=False)
    )
    first_name = cast(Optional[str], Column(String))
    last_name = cast(Optional[str], Column(String))
    email = cast(str, Column(String, nullable=False))
    organization: 'RelationshipProperty["Organization"]' = relationship(
        "Organization", back_populates="users"
    )


class Organization(Base):
    id = cast(
        str,
        Column(
            "id", UUID(), primary_key=True, server_default=text("uuid_generate_v4()")
        ),
    )
    name = cast(str, Column(String, nullable=False))
    users: 'RelationshipProperty[List["User"]]' = relationship(
        "User", back_populates="organization"
    )

Queries

When querying using filter() or order_by() along with a method on a column, add # type: ignore to the end of the lines with the column method calls.

db.session.query(User)
.filter(User.id.in_(user_ids))  # type: ignore
.order_by(User.created_at.desc())  # type: ignore

List assignment

When assigning a list of models to another model, the list will need to be explicitly cast to RelationshipProperty[List[Model]]

organization.users = cast("RelationshipProperty[List[User]]", users)

@jakebailey jakebailey added enhancement New feature or request and removed waiting for user response Requires more information from user labels Mar 16, 2021
@exhuma
Copy link

exhuma commented Aug 19, 2021

Thank you @archfear

Using cast on my models allowed me to finally get rid of so many squiggles. Still feels a bit redundant though. But at least it works.

@archfear
Copy link
Author

@exhuma I'm glad you found it useful. Hopefully it won't be necessary anymore at some point.

@Avasam
Copy link

Avasam commented Feb 11, 2022

I went the overload route, it felt cleaner since I didn't have to disable type checks for any lines:

class Player(BaseModel):
    __tablename__ = "player"

    user_id = db.Column(db.String(8), primary_key=True)
    name = db.Column(db.String(32), nullable=False)
    # The biggest region code I found so far was "us/co/coloradosprings" at 21
    country_code = db.Column(db.String(24))
    score = db.Column(db.Integer, nullable=False)
    score_details = db.Column(db.String())
    last_update = db.Column(db.DateTime())
    rank: Optional[int] = None

    schedules = db.relationship("Schedule", back_populates="owner")

    if TYPE_CHECKING:
        @overload
        def __init__(  # type: ignore
            self,
            user_id: str | Column[String],
            name: str | Column[String],
            country_code: Optional[str | Column[String]],
            score: int | float | Column[Integer],
            last_update: Optional[str | Column[DateTime]],
            score_details: Optional[str | Column[String]] = ...,
            rank: Optional[int] = ...
        ): ...

If you have a model that you need to edit/update, you still need to explicitely type the properties:

class ScheduleGroup(BaseModel):
    __tablename__ = "schedule_group"

    group_id = db.Column(db.Integer, primary_key=True)
    name: str | Column[String] = db.Column(db.String(128), nullable=False, default="")
    owner_id = db.Column(db.String(8), db.ForeignKey("player.user_id"), nullable=False)
    order: Optional[int | Column[Integer]] = db.Column(db.Integer, nullable=False, default=-1)
    if TYPE_CHECKING:
        @overload
        def __init__(  # type: ignore
            self,
            group_id: int | Column[Integer] = ...,
            name: str | Column[String] = ...,
            order: Optional[int | Column[Integer]] = ...,
            owner_id: str | Column[String] = ...,
        ): ...
    
[...]
    
schedule_group_to_update.name = name
schedule_group_to_update.order = order

It's still not perfect if you use a column method (like so: Player.country_code.like(f"{country_code}/%")) and also need to update that column directly (player.country_code = country_code). but at least it's another option which, for me, did not require disabling typing for any line other than for "__init__" is marked as overload, but no implementation is provided (which I might just configure to disable if I can)

@exhuma
Copy link

exhuma commented Feb 12, 2022

I actually switched back to mypy with the sqlalchemy-stubs plugin for projects using SA. It works better than pyright/pylance in my experience. I do prefer pyright/pylance for non-SQLAlchemy projects though because it is so much faster than mypy.

Incidentally, I see that SA now provides an installation-extra via sqlalchemy[mypy] which might be worth checking out. I have not done so yet.

@debonte
Copy link
Contributor

debonte commented Jun 30, 2022

@archfear, can this issue be closed at this point, or is there still something that you'd like the Pylance team to address while waiting for SQLAlchemy 2.0 to be released?

@archfear
Copy link
Author

archfear commented Jul 2, 2022

@debonte My team would definitely find this valuable. We're currently using the workaround I described above which effectively disables type checking for everything other that setting / getting a column. It looks like @Avasam's solution might be better than mine, but it's still pretty limited.

One interesting thing to note is that Pycharm includes their own SQLAlchemy stubs with their built in type checker. I realize that's not ideal and this should really be SQLAlchemy's problem, but it adds an additional barrier to Pylance adoption given SQLAlchemy's popularity.

I suspect that it will be a while before SQLAlchemy 2.0 is released and popular libraries have been updated to work with it.

If anyone reading this is starting a new project, I'd recommend checking out SQLModel. It combines sqlalchemy-core with Pydantic and is built with type annotations in mind.

@judej
Copy link
Contributor

judej commented Jul 6, 2022

Currently there is no way to disable the stub. The only option is to disable the stubs. Will add this as an enhancement discussion.

@microsoft microsoft locked and limited conversation to collaborators Jul 6, 2022
@judej judej converted this issue into discussion #3005 Jul 6, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
enhancement New feature or request reference
Projects
None yet
Development

No branches or pull requests

8 participants