Remove Job assert

[michelwi]

This line breaks my type checker (Pyright / Pylance) and makes it think that all instances of specific jobs are of the parent class Job. e.g.

from sisyphus import Job

class MyJob(Job):
    def __init__(self, foo):
        self.foo = foo

my_job = MyJob("foo")
print(my_job.foo)

The code works, but type checker complains that sisyphus.Job has no member foo.

Could we remove this line?

[critias]

This was just a sanity check, when I started this. I don't see how this could fail, so there should be no harm in removing it.

[albertz]

but type checker

This looks like a bug in the type checker. isinstance(job, Job) clearly does not mean that it is not a derived class.

I would not remove this assert just because of some buggy type checker.

I'm also curious whether Pyright / Pylance are maybe just misconfigured? I hesitate to believe they really get such basic thing wrong.

PyCharm gets this correct.

[michelwi]

bug in the type checker [...] PyCharm gets this correct.

consider these lines of code

1.            job = super(Job, cls).__new__(cls)
2.            assert isinstance(job, Job)
3.            job._sis_tags = tags

For line 1. both pycharm and pyright complain that 'JobSingleton' is not an instance or a subclass of 'Job' .
Both infer the type of job to be Any at this point.

Then, after the assert in line 2., in line 3. pycharm infers the type to be Any still and pyright infers Job.
(I would argue that pyright is "more correct" here)

Then once one defines a subclass of Job and creates an instance of it,

from sisyphus import Job
class Foo(Job):
    pass

foo = Foo()

pyright seems to stick with the Job type, while pycharm reads the correct class from the assignment.

From the error in line 1. I assume that the "metaclass with overridden __call__ method magic" in JobSingleton is too complicated for both type checkers. I have tried to come up with a reformulation of the JobSingleton class but was not successful yet.

[albertz]

One thing we probably should change:

class JobSingleton(type):
    def __call__(cls, *args, **kwargs):

to

class JobSingleton(type):
    def __call__(cls: Type[T], *args, **kwargs) -> T:

(E.g. check mypy doc.)

Instead of just using T = TypeVar("T"), maybe you want to use:

T = TypeVar("T", bound="Job")

Then, after the assert in line 2., in line 3. pycharm infers the type to be Any still and pyright infers Job.
(I would argue that pyright is "more correct" here)

You need to differentiate the cases when PyCharm or other type checkers think that job has exactly this type (type(job) is Job) or is inherited from it (isinstance(job, Job)). I think when PyCharm infers the type to be Any, this refers to the exact type, which is still any type, with the only additional information that it is inherited from Job.

But I'm also not sure.

Yes, in any case, metaclasses and such logic will confuse/break most/all type checkers.

But I still would be hesitant to remove a correct assert check just because some type checker is buggy/broken/incomplete.

Can't you add some comment which would make this code ignored by Pyright / Pylance? Most software would ignore it when you add noqa, like:

assert isinstance(job, Job)  # noqa

[critias]

@michelwi could you try if adding # noqa solves your problem?

I agree with @albertz that keeping the assert would be better, so it would be great if we find a solution to keep it and to solve your problem.

And yes: Having more type hinds in Sisyphus would be great and it's of cause hard for any type checker to follow that metaclasses logic...

[michelwi]

could you try if adding # noqa solves your problem?

It or any other pragma variant does not. But @albertz idea with TypeVar annotation seems to also solve my problem. I will then add the assert again together with the type hints.