⚙ D3152 Tag model entities with their "type"

douardda created this revision.May 13 2020, 5:10 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptMay 13 2020, 5:10 PM

Build has FAILED

Patch application report for D3152 (id=11193)

Rebasing onto 091498e3c7...

Current branch diff-target is up to date.

Changes applied before test

commit bbb1ada2922f3d8fccc03bed3aad516c01f9b6b7
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 13 17:03:01 2020 +0200

    Tag model entities with their "type"
    
    this aims at preventing constant usage of isinstance() based dispatch
    code when writing generic code handling model entities.
    
    For example, the "object_type" argument of JournalWriter.write_addition() has
    become superflous now we only pass model entities, etc.
    
    This idea comes olasd's reading of mypy doc:
    
      https://mypy.readthedocs.io/en/latest/literal_types.html#tagged-unions

Link to build: https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/54/
See console output for more information: https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/54/console

Harbormaster failed remote builds in B12375: Diff 11193!May 13 2020, 5:10 PM

Great! You know how little I like isinstance ;)

But I think you would need to add tag: Union[Literal["content"], Literal["directory"], ...] to BaseModel for it to work without having to repeat the Union type everywhere.

And I'm not a big fan of the name. What about object_type, as we already use this name in other places?

Build is green

Patch application report for D3152 (id=11193)

Rebasing onto 091498e3c7...

Current branch diff-target is up to date.

Changes applied before test

commit bbb1ada2922f3d8fccc03bed3aad516c01f9b6b7
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 13 17:03:01 2020 +0200

    Tag model entities with their "type"
    
    this aims at preventing constant usage of isinstance() based dispatch
    code when writing generic code handling model entities.
    
    For example, the "object_type" argument of JournalWriter.write_addition() has
    become superflous now we only pass model entities, etc.
    
    This idea comes olasd's reading of mypy doc:
    
      https://mypy.readthedocs.io/en/latest/literal_types.html#tagged-unions

See https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/55/ for more details.

Harbormaster completed remote builds in B12375: Diff 11193.May 13 2020, 5:21 PM

In D3152#76561, @vlorentz wrote:

Great! You know how little I like isinstance ;)

+1

But I think you would need to add tag: Union[Literal["content"], Literal["directory"], ...] to BaseModel for it to work without having to repeat the Union type everywhere.

When using this form of object type dispatching, we really should use an explicit Union of the supported BaseModel subclasses. Because there's very few cases where we actually support BaseModels themselves.

And I'm not a big fan of the name. What about object_type, as we already use this name in other places?

Yeah, I agree with that.

I like the idea too (except the naming, should be object_type instead of tag).

One thing to know for production deployment is that Literal and Final have been added in typing-extensions 3.7.4
and version packaged in buster is 3.7.2.

This means typing-extensions package from buster-backports must be used.

swh/model/model.py
93	I would rather use `Final` from typing-extensions to avoid repeating the literal string. tag: Final = "person"

ardumont added a subscriber: ardumont.May 13 2020, 7:53 PM

ardumont added inline comments.

swh/model/model.py

93

That's even more than that, that will even make it unmodifiable [1]. We can also adds its type Final[str].

So that suits us even better ;)

[1] Quoting the linked documentation:

You can use the typing_extensions. Final qualifier to indicate that a name or attribute should not be reassigned, redefined, or overridden. This is often useful for module and class level constants as a way to prevent unintended modification. Mypy will prevent further assignments to final names...

I like the general idea and i think @anlambert's proposal improvment is even better ;)

douardda added inline comments.May 14 2020, 10:46 AM

swh/model/model.py
93	The advantage of Literal is it's a PEP (and has been integrated in py3.8). I have no strong opinion on this myself, I'll do whatever we decide.

In D3152#76561, @vlorentz wrote:

And I'm not a big fan of the name. What about object_type, as we already use this name in other places?

yes I also agree, but not a big fan of object_type either. However I have no better candidate to propose, I'm afraid. entity_type maybe ?

anlambert added inline comments.May 14 2020, 11:31 AM

swh/model/model.py
93	`Final` is also a PEP and has also been integrated in Python 3.8 ;-)

In D3152#76595, @douardda wrote:

In D3152#76561, @vlorentz wrote:

And I'm not a big fan of the name. What about object_type, as we already use this name in other places?

yes I also agree, but not a big fan of object_type either. However I have no better candidate to propose, I'm afraid. entity_type maybe ?

I'm not sure "entity" a better name than "object". Plus, we already use "object" everywhere, so we would have to change that.

If we really want to move away from "object", I think it should be "artifact", to be consistent with https://docs.softwareheritage.org/devel/swh-model/data-model.html#software-artifacts

Hmm actually, "artifact" isn't great either, because we'll want to use it to mean extrinsic metadata , which aren't artifacts. Ugh...

object_type is used in the swh.model.identifiers.PersistentId in charge of
resolving SWHIDs now. It's also what we used in the journal "currently".

As we do not seem to find a better name anyway, the least friction path would
be consistency. My 2 cents.

If someone has better suggestions though, i'll comply with the team decision.

Naming is so hard...

I think we're pretty much set with object_type in the current lingo. It's very unspecific, but then again there's a bunch of different things that end up as model objects. So I suggest we keep using that.

I agree with the suggestion of using Final instead of Literal[foo].

From PEP 586:

The Final qualifier serves as a shorthand for declaring that a variable is effectively Literal.

We should also have a test checking that no two objects types have the same object_type value. This test should check all BaseModel subclasses (instead of just stuff generated by strategies.objects).

rename tag as object_type and use a Final instead of a Literal.

so be it :-)

Build is green

Patch application report for D3152 (id=11204)

Rebasing onto cce3036634...

First, rewinding head to replay your work on top of it...
Applying: Tag model entities with their "object_type"

Changes applied before test

commit 9b82c6036596d7a03b5035275fd7d2fff1e9e093
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 13 17:03:01 2020 +0200

    Tag model entities with their "object_type"
    
    this aims at preventing constant usage of isinstance() based dispatch
    code when writing generic code handling model entities.
    
    For example, the "object_type" argument of JournalWriter.write_addition() has
    become superflous now we only pass model entities, etc.
    
    This idea comes olasd's reading of mypy doc:
    
      https://mypy.readthedocs.io/en/latest/literal_types.html#tagged-unions

See https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/56/ for more details.

Harbormaster completed remote builds in B12392: Diff 11204.May 15 2020, 9:58 AM

Looks good.

Do you plan to add the test olasd mentioned? [1]

We should also have a test checking that no two objects types have the same object_type value. This test should check all BaseModel subclasses (instead of just stuff generated by strategies.objects).

[1] D3152#76613

In D3152#76660, @ardumont wrote:

Looks good.

Do you plan to add the test olasd mentioned? [1]

We should also have a test checking that no two objects types have the same object_type value. This test should check all BaseModel subclasses (instead of just stuff generated by strategies.objects).

[1] D3152#76613

I'm on it

add the test suggested by olasd

Build is green

Patch application report for D3152 (id=11205)

Rebasing onto cce3036634...

First, rewinding head to replay your work on top of it...
Applying: Tag model entities with their "object_type"

Changes applied before test

commit 573cf20638c9bd8528da1476a6a149ff6ed562b5
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 13 17:03:01 2020 +0200

    Tag model entities with their "object_type"
    
    this aims at preventing constant usage of isinstance() based dispatch
    code when writing generic code handling model entities.
    
    For example, the "object_type" argument of JournalWriter.write_addition() has
    become superflous now we only pass model entities, etc.
    
    This idea comes olasd's reading of mypy doc:
    
      https://mypy.readthedocs.io/en/latest/literal_types.html#tagged-unions

See https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/57/ for more details.

Harbormaster completed remote builds in B12393: Diff 11205.May 15 2020, 10:18 AM

assert object_type is unset is the class has subclasses

same, but with the actual chuck

Build is green

Patch application report for D3152 (id=11206)

Rebasing onto cce3036634...

First, rewinding head to replay your work on top of it...
Applying: Tag model entities with their "object_type"

Changes applied before test

commit e84d636591b3d7eb9f35921661ddc2675ffc37f9
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 13 17:03:01 2020 +0200

    Tag model entities with their "object_type"
    
    this aims at preventing constant usage of isinstance() based dispatch
    code when writing generic code handling model entities.
    
    For example, the "object_type" argument of JournalWriter.write_addition() has
    become superflous now we only pass model entities, etc.
    
    This idea comes olasd's reading of mypy doc:
    
      https://mypy.readthedocs.io/en/latest/literal_types.html#tagged-unions

See https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/58/ for more details.

Harbormaster completed remote builds in B12394: Diff 11206.May 15 2020, 10:21 AM

Build has FAILED

Patch application report for D3152 (id=11207)

Rebasing onto cce3036634...

First, rewinding head to replay your work on top of it...
Applying: Tag model entities with their "object_type"

Changes applied before test

commit c919677105f9048c28749ddede877feaaafe34c3
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 13 17:03:01 2020 +0200

    Tag model entities with their "object_type"
    
    this aims at preventing constant usage of isinstance() based dispatch
    code when writing generic code handling model entities.
    
    For example, the "object_type" argument of JournalWriter.write_addition() has
    become superflous now we only pass model entities, etc.
    
    This idea comes olasd's reading of mypy doc:
    
      https://mypy.readthedocs.io/en/latest/literal_types.html#tagged-unions

Link to build: https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/59/
See console output for more information: https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/59/console

Harbormaster failed remote builds in B12395: Diff 11207!May 15 2020, 10:23 AM

Looks good (remains to unstuck the build ;)

This revision is now accepted and ready to land.May 15 2020, 10:44 AM

douardda mentioned this in T2422: Add an `object_type` attribute to model classes.May 26 2020, 5:03 PM

douardda added a task: T2422: Add an `object_type` attribute to model classes.

douardda mentioned this in D3270: Introduce some scaffolding for an attrs-based BaseSchedulerModel.Jun 12 2020, 2:07 PM

Update the diff to make it work (see T2422)

Build is green

Patch application report for D3152 (id=11866)

Rebasing onto 661b7c2c76...

Current branch diff-target is up to date.

Changes applied before test

commit 0678fc766e535c0beb1b292e63de04d1b05c37e4
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 13 17:03:01 2020 +0200

    Tag model entities with their "object_type"
    
    this aims at preventing constant usage of isinstance() based dispatch
    code when writing generic code handling model entities.
    
    For example, the "object_type" argument of JournalWriter.write_addition() has
    become superflous now we only pass model entities, etc.
    
    This idea comes olasd's reading of mypy doc:
    
      https://mypy.readthedocs.io/en/latest/literal_types.html#tagged-unions

See https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/80/ for more details.

Harbormaster completed remote builds in B13081: Diff 11866.Jun 24 2020, 4:02 PM

improve commit message + use '_' in DiskBakedContent.object_type

instead of '-', for consistency.

Build is green

Patch application report for D3152 (id=11870)

Rebasing onto 661b7c2c76...

Current branch diff-target is up to date.

Changes applied before test

commit 82818999d215e2ffc027174eafb4c0e906b714db
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 13 17:03:01 2020 +0200

    Tag model entities with their "object_type"
    
    this aims at preventing constant usage of isinstance() based dispatch
    code when writing generic code handling model entities.
    
    For example, the "object_type" argument of JournalWriter.write_addition() has
    become superflous now we only pass model entities, etc.
    
    This idea comes olasd's reading of mypy doc:
    
      https://mypy.readthedocs.io/en/latest/literal_types.html#tagged-unions
    
    This comes with a refactoring of from_dict.DiskBackedContent to make
    it *not* inherit from model.Content: object_type being Final, it cannot
    be overloaded.

See https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/81/ for more details.

Harbormaster completed remote builds in B13084: Diff 11870.Jun 24 2020, 4:34 PM

due to changes in from_disk, I'd rather have a new review on this.

I don't see any issues with this ;)
I'll let others review as well.

swh/model/from_disk.py
50 ↗	(On Diff #11870)	Is that to exclude it from the comparison computation?
swh/model/model.py
1	copyright header ¯\_(ツ)_/¯

This revision is now accepted and ready to land.Jun 24 2020, 5:00 PM

douardda added inline comments.Jun 24 2020, 5:37 PM

swh/model/from_disk.py
50 ↗	(On Diff #11870)	the `eq=False`? yes it is. I've introduced this a few days ago because this ctime attribute really is just a helper metadata, not an "intrisinc property" of a Content. See a95646fc
swh/model/model.py
1	thx

update copyright

Build is green

Patch application report for D3152 (id=11872)

Rebasing onto 661b7c2c76...

Current branch diff-target is up to date.

Changes applied before test

commit e632abed41c5cd71507c4222e1e85987556b232c
Author: David Douard <david.douard@sdfa3.org>
Date:   Wed May 13 17:03:01 2020 +0200

    Tag model entities with their "object_type"
    
    this aims at preventing constant usage of isinstance() based dispatch
    code when writing generic code handling model entities.
    
    For example, the "object_type" argument of JournalWriter.write_addition() has
    become superflous now we only pass model entities, etc.
    
    This idea comes olasd's reading of mypy doc:
    
      https://mypy.readthedocs.io/en/latest/literal_types.html#tagged-unions
    
    This comes with a refactoring of from_dict.DiskBackedContent to make
    it *not* inherit from model.Content: object_type being Final, it cannot
    be overloaded.

See https://jenkins.softwareheritage.org/job/DMOD/job/tests-on-diff/82/ for more details.

Harbormaster completed remote builds in B13086: Diff 11872.Jun 24 2020, 5:41 PM

Closed by commit rDMODe632abed41c5: Tag model entities with their "object_type" (authored by douardda). · Explain WhyJun 25 2020, 10:29 AM

This revision was automatically updated to reflect the committed changes.

douardda added a commit: rDMODe632abed41c5: Tag model entities with their "object_type".

ardumont mentioned this in D3367: Migrate to use the object's object_type field when computing objects.Jun 29 2020, 11:35 AM

ardumont mentioned this in D3368: Migrate to Use the object's object_type field when computing objects.Jun 29 2020, 11:47 AM

ardumont mentioned this in D3373: webapp: Migrate to Use the object's object_type field.Jun 30 2020, 11:24 AM

ardumont mentioned this in rDLDBASE62aad2943238: Migrate to use object's "object_type" field when computing objects.Jun 30 2020, 11:51 AM

ardumont mentioned this in rDLDSVNb268749fdaf5: Migrate to Use the object's object_type field when computing objects.Jun 30 2020, 11:54 AM

ardumont mentioned this in rDWAPPSf433684afd32: Migrate to Use the object's object_type field.Jun 30 2020, 6:21 PM

Tag model entities with their "type"
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Patch application report for D3152 (id=11193)

Changes applied before test

Patch application report for D3152 (id=11193)

Changes applied before test

Patch application report for D3152 (id=11204)

Changes applied before test

Patch application report for D3152 (id=11205)

Changes applied before test

Patch application report for D3152 (id=11206)

Changes applied before test

Patch application report for D3152 (id=11207)

Changes applied before test

Patch application report for D3152 (id=11866)

Changes applied before test

Patch application report for D3152 (id=11870)

Changes applied before test

Patch application report for D3152 (id=11872)

Changes applied before test

Revision Contents
Changeset List

Diff 11206

requirements.txt

swh/model/model.py

swh/model/tests/test_model.py

Tag model entities with their "type"ClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D3152 (id=11193)

Changes applied before test

Patch application report for D3152 (id=11193)

Changes applied before test

Patch application report for D3152 (id=11204)

Changes applied before test

Patch application report for D3152 (id=11205)

Changes applied before test

Patch application report for D3152 (id=11206)

Changes applied before test

Patch application report for D3152 (id=11207)

Changes applied before test

Patch application report for D3152 (id=11866)

Changes applied before test

Patch application report for D3152 (id=11870)

Changes applied before test

Patch application report for D3152 (id=11872)

Changes applied before test

Revision ContentsChangeset List

Diff 11206

requirements.txt

swh/model/model.py

swh/model/tests/test_model.py

Tag model entities with their "type"
ClosedPublic
Actions

Revision Contents
Changeset List