Page MenuHomeSoftware Heritage
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:

  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.

Diff Detail

Repository
rDMOD Data model
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17933
Build 27705: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 27704: arc lint + arc unit

Unit TestsFailed
View All

TimeTest
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.model.tests.test_from_disk.FileToContent::test_file_to_content_model
self = <swh.model.tests.test_from_disk.FileToContent testMethod=test_file_to_content_model> def test_file_to_content_model(self):
4 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.model.tests.test_from_disk.TarballIterDirectory::test_iter_directory
self = <swh.model.tests.test_from_disk.TarballIterDirectory testMethod=test_iter_directory> def test_iter_directory(self):
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.model.tests.test_from_disk.TestDiskBackedContent::test_lazy_data
self = <swh.model.tests.test_from_disk.TestDiskBackedContent testMethod=test_lazy_data> def test_lazy_data(self):
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.model.tests.test_from_disk.TestDiskBackedContent::test_missing_path
self = <swh.model.tests.test_from_disk.TestDiskBackedContent testMethod=test_missing_path> def test_missing_path(self):
1 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.model.tests.test_from_disk.TestDiskBackedContent::test_with_data
self = <swh.model.tests.test_from_disk.TestDiskBackedContent testMethod=test_with_data> def test_with_data(self):
View Full Test Results (6 Failed · 289 Passed · 2 Skipped)

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
swh-public-ci added a comment.Dec 14 2020, 5:40 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
vlorentz updated this revision to Diff 16795.Dec 15 2020, 2:10 PM

fix tests

swh-public-ci added a comment.Dec 15 2020, 2:11 PM

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
ardumont added a subscriber: ardumont.EditedDec 16 2020, 6:26 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

ardumont accepted this revision as: ardumont.EditedDec 16 2020, 6:26 PM

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
vlorentz added a comment.Dec 17 2020, 10:52 AM
In D4736#118736, @ardumont wrote:

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

All tests still pass

vlorentz added a comment.Dec 17 2020, 10:55 AM
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
ardumont added a comment.EditedDec 17 2020, 4:56 PM

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)

vlorentz added a comment.Dec 18 2020, 1:36 PM

I checked swh-journal and swh-storage.

ardumont accepted this revision.Dec 18 2020, 1:38 PM

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..

Revision Contents
Changeset List

PathSize
swh/
model/
38 lines

Diff 16773

View Options

swh/model/model.py

Loading...
About · Developer information · Legal