Differential D4736

model: Make all classes slotted.
ClosedPublic
Actions

Authored by vlorentz on Dec 14 2020, 5:38 PM.

Details

Reviewers

ardumont

Group Reviewers

Reviewers

Commits

rDMOD76b744e08fbf: model: Make all classes slotted.

Summary

Unfortunately, sphinx (actually, autodoc) only picks up attributes if
they fall in any of these cases:

are enum variants
are in slots
are in dict
have an annotation
are found using its custom parser

(see get_object_members in sphinx/ext/autodoc/importer.py)

In theory, option 5 should work for us; unfortunately, autodoc only
asks the parser the list of members with a comment.
And it's not easy to adapt it to ask the parser for all members,
because said parser (sphinx/pycode/parser.py) does not return the class
qualname (aka. namespace) for members without comments.

So, as I don't want to change the interface of sphinx.pycode.parser,
this commit switches to relying on option 3, by adding slots for
all attr classes.

Additionally, this might have some performance/memory improvement
(though I did not check) and will further avoid mutation of these
objects.

Diff Detail

Repository

rDMOD Data model

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vlorentz created this revision.Dec 14 2020, 5:38 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptDec 14 2020, 5:38 PM

Build has FAILED

Patch application report for D4736 (id=16773)

Rebasing onto a3b6a64480...

Current branch diff-target is up to date.

Changes applied before test

commit 8d1b6a8dc519f154d9fa97b939e7476b3e5b0caa
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Mon Dec 14 17:38:29 2020 +0100

    model: Make all classes slotted.
    
    Unfortunately, sphinx (actually, autodoc) only picks up attributes if
    they fall in any of these cases:
    
    1. are enum variants
    2. are in slots
    3. are in __dict__
    4. have an annotation
    5. are found using its custom parser
    
    (see get_object_members in sphinx/ext/autodoc/importer.py)
    
    In theory, option 5 should work for us; unfortunately, autodoc only
    asks the parser the list of members with a comment.
    And it's not easy to adapt it to ask the parser for all members,
    because said parser (sphinx/pycode/parser.py) does not return the class
    qualname (aka. namespace) for members without comments.
    
    So, as I don't want to change the interface of sphinx.pycode.parser,
    this commit switches to relying on option 3, by adding __slots__ for
    all attr classes.
    
    Additionally, this might have some performance/memory improvement
    (though I did not check) and will further avoid mutation of these
    objects.

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

Harbormaster failed remote builds in B17933: Diff 16773!Dec 14 2020, 5:40 PM

fix tests

Build is green

Patch application report for D4736 (id=16795)

Rebasing onto a3b6a64480...

Current branch diff-target is up to date.

Changes applied before test

commit 76b744e08fbf37d5a5313987b1e7a7ad1870125d
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Mon Dec 14 17:38:29 2020 +0100

    model: Make all classes slotted.
    
    Unfortunately, sphinx (actually, autodoc) only picks up attributes if
    they fall in any of these cases:
    
    1. are enum variants
    2. are in slots
    3. are in __dict__
    4. have an annotation
    5. are found using its custom parser
    
    (see get_object_members in sphinx/ext/autodoc/importer.py)
    
    In theory, option 5 should work for us; unfortunately, autodoc only
    asks the parser the list of members with a comment.
    And it's not easy to adapt it to ask the parser for all members,
    because said parser (sphinx/pycode/parser.py) does not return the class
    qualname (aka. namespace) for members without comments.
    
    So, as I don't want to change the interface of sphinx.pycode.parser,
    this commit switches to relying on option 3, by adding __slots__ for
    all attr classes.
    
    Additionally, this might have some performance/memory improvement
    (though I did not check) and will further avoid mutation of these
    objects.

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

Harbormaster completed remote builds in B17954: Diff 16795.Dec 15 2020, 2:11 PM

lgtm (i had to check the doc [1])

Can you please double check it does not break serialization (for some reason)?

[1] https://www.attrs.org/en/stable/glossary.html#term-slotted-classes

Only accepted as me so others might want to join the party.

This revision is now accepted and ready to land.Dec 16 2020, 6:26 PM

In D4736#118736, @ardumont wrote:

Can you please double check it does not break serialization (for some reason)?

All tests still pass

In D4736#118737, @ardumont wrote:

Only accepted as me so others might want to join the party.

That's not how it works for Phab, only one accept is required to consider the diff ready to land

vlorentz added 1 blocking reviewer(s): Reviewers.Dec 17 2020, 10:55 AM

This revision now requires review to proceed.Dec 17 2020, 10:55 AM

Can you please double check it does not break serialization (for some reason)?

All tests still pass

And are you 100% sure that's enough in regards to other modules?
I'm talking about potential effects on other modules especially the rpc serialization layers (journal, rpc backends)

I checked swh-journal and swh-storage.

great, thanks

This revision is now accepted and ready to land.Dec 18 2020, 1:38 PM

Closed by commit rDMOD76b744e08fbf: model: Make all classes slotted. (authored by vlorentz). · Explain WhyDec 18 2020, 1:52 PM

This revision was automatically updated to reflect the committed changes.

vlorentz added a commit: rDMOD76b744e08fbf: model: Make all classes slotted..