Avoid string types with ClassVar

[rra]

As of Python 3.9.8, this does not work with Pydantic BaseModel, and
does not work in a way that causes the whole module to fail to
import. See pydantic/pydantic#3401.

In general, Pydantic does not work well with string types as enabled
by from __future__ import annotations, which is why making that
feature the default has been delayed. See
https://lwn.net/Articles/858576/ for more information.

Fix this by removing from __future__ import annotations from the
files using ClassVar with Pydantic and explicitly quote the type
arguments that contain forward references (all of which are outside
the scope of what Pydantic cares about).

Checklist

• ran Jenkins
• added a release note for user-visible changes to doc/changes

[timj]

Since this seems to be a fundamental problem with class methods, is there are way to tell the annotation system that the thing returned is an instance of the cls ? That would seem to be the right solution. I'd like @TallJimbo to comment on this PR since he's the typing expert for butler.

[natelust]

I think you use

T = TypeVar("T")
def from_records(cls: T, record: LogRecord) -> T:

But I don't know how that will play with pydandic either

[natelust]

oh oh i was not thinking properly, T is probably the metaclass of whatever cls is in this case. It might be possible to annotate is as cls: Type[T] but I would need to test that out.

[rra]

The technically correct solution, which supports returning subclass instances, is:

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

class ButlerLogRecord:
    @classmethod
    def from_record(cls, record: LogRecord) -> T

but I suspect you won't like that much better. :)

See python/typing#58 for discussion of this. It doesn't look like there are any plans to add a simpler method. The quoted string approach is the upstream recommendation for most code where the type bound is not that interesting.

[natelust]

I think you do still need TypeVar[T] before cls (at least that is what the link you posted seems to suggest)

[rra]

Yes, indeed, otherwise it's not bound. I missed that part of the setup.

[natelust]

I'm happy to see my guess at this turned out to be what was recommended (sans the bind part, which one could take or leave in this case, as I dont think it would matter). FWIW I actually have used this in places in place of explicit type to future proof against inheritance and think we should probably do it this way.

[TallJimbo]

This makes me sad, but I don't see that we have much choice. from __future__ import annotations is IMO really critical for making type annotations usable in some contexts, so I'd like to make sure any future accomodations for pydantic involve putting the pydantic models into their own module so we don't lose that future-import anywhere we don't have to. But I think this change is largely consistent with that.

Also, on how to type classmethod returns, I'm surprised that this example from @rra :

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

class ButlerLogRecord:
    @classmethod
    def from_record(cls, record: LogRecord) -> T:

works at all, unless T is otherwise referenced by this class or method. Anyway, if it does work, I'm very curious about what it's actually saying, and I'm a little suspicious that it's an accident that it works.

I do know that if you add a different annotation on cls:

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

class ButlerLogRecord:
    @classmethod
    def from_record(cls: Type[T], record: LogRecord) -> T:

then this says something different: this method returns ButlerLogRecord and any subclass override returns an instance of that subclass (not some other subclass of the same base class, and not a direct base class instance).

Overall, I think using the actual class name with no generics and from __future__ import annotations is almost always the right call, unless you really do want to require most-derived instances (but that's a little weird if you think about it), or...pydantic takes that option away from you.

A side note: @timj mentioned that we should spend the holiday break coding something fun and far-out, if we spent it working at all. Between this, the ugly validation-performance workarounds @natelust is adding on DM-30266, and the comparatively pleasant experience of working with serde (and petgraph), I'm thinking that some RIIR on butler primitives, containers-thereof, and QGs is sounding fun...

[TallJimbo]

(made a couple of fixes to the code blocks above; apologies for not getting them right the first time around)

[natelust]

@TallJimbo I think you missed the discussion on needing Type, but yes we came to the same conclusion

[codecov]

Codecov Report

Merging #602 (90b8334) into master (d67b257) will increase coverage by 0.00%.
The diff coverage is 85.71%.

@@           Coverage Diff           @@
##           master     #602   +/-   ##
=======================================
  Coverage   83.53%   83.54%           
=======================================
  Files         241      241           
  Lines       30318    30316    -2     
  Branches     4528     4528           
=======================================
- Hits        25327    25326    -1     
+ Misses       3796     3795    -1     
  Partials     1195     1195           

Impacted Files	Coverage Δ
python/lsst/daf/butler/core/serverModels.py	0.00% <0.00%> (ø)
python/lsst/daf/butler/core/logging.py	96.15% <100.00%> (-0.02%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d67b257...90b8334. Read the comment docs.

[TallJimbo]

(made a couple of fixes to the code blocks above; apologies for not getting them right the first time around)