Fix Channel type hint issues and update strawberry version to 0.190.0

[rjwills28]

It would be worth us updating to the latest version of Strawberry was it contains a fix for the KeyError exceptions that we see when unsubscribing. The DeprecationWarnings described in #72 have also been fixed in this version.

Our pytests pass with this new version and a quick run of the performance tests on my local machine gave consisted performance.

The only problem I came across was tighter mypy type hinting checks. Some of our fields were missing type hints. A non-trivial one to solve was:
float = strawberry.field(resolver=resolve_float)
as float is an inbuilt Python type and it looks like we're trying to override is here. The format below causes the pytests to fail:
float: float = strawberry.field(resolver=resolve_float)
Instead I have had to create an alias for the float type and refer to that for the type hint.

The other problem I had was with errors from this type hinting
getChannel: Channel = strawberry.field(resolver=get_channel)
After several attempts I was unable to fix this so decided to add the flag to ignore this line. Perhaps not the best workaround but I could not find another way.

[AlexanderWells-diamond]

What version of mypy are you using? Using 1.2.0 I don't see an error on that getChannel typing - i.e. I can remove the ignore and I see no error.

[rjwills28]

You can see the error in the CI linting after the first commit (where I only changed the Strawberry version): https://github.com/DiamondLightSource/coniql/actions/runs/5403335216/jobs/9815987842#step:4:34

The CI looks to be using v1.4.1 but it looks like it was also using this version on previous, successful lint job. That means that Strawberry must have dropped some support of mypy type hinting. I can look into this but how bother are we about that? Would that stop us updating the Strawberry version?

[AlexanderWells-diamond]

Ah I see, it was me who had the older version! Updating and I do now see the error:

src/coniql/strawberry_schema.py:177: error: Incompatible types in assignment (expression has type "coniql.types.Channel", variable has type "coniql.strawberry_schema.Channel")  [assignment]

So the issue is we have a type Channel in both the strawberry_schema and the types file. We can fix this mypy error by changing the return type of the get_channel function from TypeChannel -> Channel, which makes intuitive sense to me as this file should be referencing the Schema types, not the "internal" types.

What I don't really understand is what the Channel in types is, or what it is trying to achieve. Can you explain that to me?

[rjwills28]

But doing that gives a different mypy error:
src/coniql/strawberry_schema.py:171: error: Incompatible return value type (got "GetChannel", expected "Channel") [return-value]

The Channel in types is the base class that is used by the plugins (e.g. caplugin creates a CAChannel based off of types.Channel, whereas strawberry_schema.Channel is the Strawberry/GraphQL compatible version with the Strawberry.fields. I agree, it is a bit confusing.

[AlexanderWells-diamond]

I think we have a logical error here?

As you've done in the diff, the type we want to return is indeed the strawberry_schema Channel type. However, the function we're providing to get that object is actually returning the types Channel type. So that means whenever that we subscribe to an object, we're getting the types type of Channel, and not the strawberry_schema type.

I see that we then define resolvers for each and every parameter in the schema's Channel, which is I suppose how we handle the "wrong" type being given to the Query - we never actually try to access anything directly, we just resolve all the variables through another layer of functions (which all depend on the Channel actually being the types version, not the strawberry_schema version).

I'm not exactly sure what I want to do to fix this, if anything. Certainly it is very confusing to have two things called a Channel. But I do also note we have similar typing issues for the Subscription as we do for this Query - it also depends on the "wrong" type of Channel. Is it viable to make one of the Channel classes just extend the other? That would solve our typing issue, but may well just be more confusing...

[rjwills28]

I agree, it would be nice to clean this up for future developers.

I have had a go at reworking this so that we can create a strawberry.Channel from the types.Channel/CAChannel. I think this makes things a little clearer to read? It fixes the type hinting errors as all return types are now consistent and obvious. I had to move some methods around though, which might make this a bit tricky to review changes.

@AlexanderWells-diamond - this is just an idea of how we could clean things up, what do you think? I can also pull this out into a separate PR if that's easier but just wanted some feedback as to whether this is the way to go.

[AlexanderWells-diamond]

I think this is the right way to go. It makes the most sense to me to have the right types for the Query object.

I'm a little unhappy that there's still two classes named Channel, but that's probably not worth the time to figure out further.

Only possible improvement I can see is to shift the definitions of the various Channel classes in strawberry_schema.py to the top of the file, which would allow us to remove the quoted types, but that's so minor I'm happy with this as-is.

[rjwills28]

Thanks for the feedback. I could change the name of strawberry.Channel to ease confusion or is that not worth doing?

I did look at moving the Channel bits further up to fix that but the problem is that Channel uses ChannelValue, ChannelTime etc type hints, so we would then need to put those in quotes.

Is this all OK in one PR or shall I split it out?

[AlexanderWells-diamond]

I'm fine with this all in this PR, if you change the title a bit to reflect the shuffling of types around.

Fair point about moving the definition up.

If you want to change it go ahead, but I'm not bothered either way. I think enough work has been done on what should have been a pretty trivial change already!

[rjwills28]

Agreed, thanks for your help again @AlexanderWells-diamond