Page MenuHomeSoftware Heritage
Differential D5935

Make swh.graph dependency optional
ClosedPublic
Actions

Authored by ardumont on Jun 28 2021, 3:38 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T843: Vault: Add a "git bare" tarball cooker
T3412: Deploy vault "git bare" tarball cooker (swh-vault v0.6)
Commits
rDVAU9d0df34d412a: Make swh.graph dependency optional
Summary

It's not packaged nor ready yet as some internal reworking has been discussed.

Related to T3412

Test Plan

tox

Diff Detail

Event Timeline

ardumont created this revision.Jun 28 2021, 3:38 PM
swh-public-ci added a comment.Jun 28 2021, 3:40 PM

Build is green

Patch application report for D5935 (id=21309)

Rebasing onto 0bec623156...

Current branch diff-target is up to date.
Changes applied before test
commit a97cd94fac73a76bec59b955c8bd9a132a65b8ab
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jun 28 15:36:26 2021 +0200

    Make swh.graph dependency optional
    
    It's not packaged nor ready yet as some internal reworking has been discussed.
    
    Related to T3412

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

Harbormaster completed remote builds in B22290: Diff 21309.Jun 28 2021, 3:40 PM
ardumont requested review of this revision.Jun 28 2021, 3:40 PM
vlorentz requested changes to this revision.Jun 28 2021, 3:52 PM
vlorentz added a subscriber: vlorentz.

I would rather do this:

if "graph" in vcfg:
    from swh.graph.client import RemoteGraphClient
    graph = RemoteGraphClient(**vcfg["graph"])
else:
    graph = None

So that it errors if it's configured to use it but isn't available

This revision now requires changes to proceed.Jun 28 2021, 3:52 PM
ardumont added a comment.Jun 28 2021, 4:01 PM
In D5935#152075, @vlorentz wrote:

I would rather do this:

if "graph" in vcfg:
    from swh.graph.client import RemoteGraphClient
    graph = RemoteGraphClient(**vcfg["graph"])
else:
    graph = None

So that it errors if it's configured to use it but isn't available

ack, that was my first implementation but i recall we did use the try except with import error instead...
I'll keep the ternary form in the affectation though so mypy is still happy with the Optional[RemoteGraphClient]
to avoid the conundrum otherwise.

ardumont updated this revision to Diff 21312.EditedJun 28 2021, 4:02 PM

Adapt according to irc discussion [1] and review here.

I may still need to update the tests with those corner case...

[1]

15:42 <+olasd> you'll need an update to requirements-swh.txt too to make the dependency an extra
15:43 <+swhbot> icinga PROBLEM: service disk / on worker01.softwareheritage.org is WARNING: DISK WARNING - free space: / 6257 MB (18% inode=71%);
15:43 <+olasd> and probably test the new logic
15:43 <+olasd> I'd also replace `graph = RemoteGraphClient(**vcfg["graph"]) if "graph" in vcfg else None` with `graph = RemoteGraphClient(**vcfg["graph"]) if vcfg.get("graph") else None` to allow setting `graph: null` in the config
swh-public-ci added a comment.Jun 28 2021, 4:03 PM

Build has FAILED

Patch application report for D5935 (id=21312)

Rebasing onto 0bec623156...

Current branch diff-target is up to date.
Changes applied before test
commit cd6ef1b2629f8246835ddb69e4094ce32611e88c
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jun 28 15:36:26 2021 +0200

    Make swh.graph dependency optional
    
    It's not packaged nor ready yet as some internal reworking has been discussed.
    
    Related to T3412

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

Harbormaster failed remote builds in B22293: Diff 21312!Jun 28 2021, 4:03 PM
ardumont added a comment.Jun 28 2021, 4:05 PM

Build has FAILED

16:03:42  swh/vault/cookers/git_bare.py:30: error: Skipping analyzing "swh.graph.client": found module but no type hints or library stubs
16:03:42  swh/vault/cookers/__init__.py:94: error: Skipping analyzing "swh.graph.client": found module but no type hints or library stubs
16:03:42  swh/vault/cookers/__init__.py:94: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
16:03:42  swh/vault/tests/test_git_bare_cooker.py:21: error: Skipping analyzing "swh.graph.naive_client": found module but no type hints or library stubs
16:03:42  swh/vault/cli.py:104: error: Skipping analyzing "swh.graph.client": found module but no type hints or library stubs
16:03:42  Found 4 errors in 4 files (checked 33 source files)

Now what!?

ardumont added a comment.EditedJun 28 2021, 5:02 PM

Build has FAILED

16:03:42  swh/vault/cookers/git_bare.py:30: error: Skipping analyzing "swh.graph.client": found module but no type hints or library stubs
16:03:42  swh/vault/cookers/__init__.py:94: error: Skipping analyzing "swh.graph.client": found module but no type hints or library stubs
16:03:42  swh/vault/cookers/__init__.py:94: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
16:03:42  swh/vault/tests/test_git_bare_cooker.py:21: error: Skipping analyzing "swh.graph.naive_client": found module but no type hints or library stubs
16:03:42  swh/vault/cli.py:104: error: Skipping analyzing "swh.graph.client": found module but no type hints or library stubs
16:03:42  Found 4 errors in 4 files (checked 33 source files)

Now what!?

Well, that game is not funny.
I'll keep the first implementation and log a warning when graph is configured
but the module is not found so we have the best of both suggestions.

ardumont updated this revision to Diff 21314.Jun 28 2021, 5:03 PM

Do as explained

swh-public-ci added a comment.Jun 28 2021, 5:06 PM

Build has FAILED

Patch application report for D5935 (id=21314)

Rebasing onto 0bec623156...

Current branch diff-target is up to date.
Changes applied before test
commit 71a160b97c34089f5fc6c650627c17c72f2ff854
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jun 28 15:36:26 2021 +0200

    Make swh.graph dependency optional
    
    It's not packaged nor ready yet as some internal reworking has been discussed.
    
    Related to T3412

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

Harbormaster failed remote builds in B22295: Diff 21314!Jun 28 2021, 5:06 PM
vlorentz added a comment.Jun 28 2021, 5:06 PM
In D5935#152078, @ardumont wrote:

ack, that was my first implementation but i recall we did use the try except with import error instead...

Where? was it for something that was explicitly configured too?

I'll keep the ternary form in the affectation though so mypy is still happy with the Optional[RemoteGraphClient]
to avoid the conundrum otherwise.

What? You can do this to please mypy:

graph: Optional[RemoteGraphClient]
if "graph" in vcfg:
    from swh.graph.client import RemoteGraphClient
    graph = RemoteGraphClient(**vcfg["graph"])
else:
    graph = None
ardumont added a comment.Jun 28 2021, 5:08 PM
In D5935#152112, @vlorentz wrote:
In D5935#152078, @ardumont wrote:

ack, that was my first implementation but i recall we did use the try except with import error instead...

Where? was it for something that was explicitly configured too?

locally, prior to opening the diff so you did not see it.

I'll keep the ternary form in the affectation though so mypy is still happy with the Optional[RemoteGraphClient]
to avoid the conundrum otherwise.

What? You can do this to please mypy:

graph: Optional[RemoteGraphClient]
if "graph" in vcfg:
    from swh.graph.client import RemoteGraphClient
    graph = RemoteGraphClient(**vcfg["graph"])
else:
    graph = None

No, it's not happy because the first implementation is actually a RemoteGraphClient and not an optional one.
And prior to the if, there is no symbol RemoteGraphClient yet so we cannot here.

ardumont updated this revision to Diff 21319.Jun 28 2021, 5:35 PM

Adapt according to discussions

swh-public-ci added a comment.Jun 28 2021, 5:37 PM

Build has FAILED

Patch application report for D5935 (id=21319)

Rebasing onto 0bec623156...

Current branch diff-target is up to date.
Changes applied before test
commit 6b5d173cb6ef22d81f27a12ec15a8d3e8fda5a3d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jun 28 15:36:26 2021 +0200

    Make swh.graph dependency optional
    
    It's not packaged nor ready yet as some internal reworking has been discussed.
    
    Related to T3412

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

Harbormaster failed remote builds in B22300: Diff 21319!Jun 28 2021, 5:37 PM
ardumont added a comment.Jun 28 2021, 5:40 PM

Finally, what i forgot to say earlier:

if "graph" in vcfg:
    from swh.graph.client import RemoteGraphClient
    graph = RemoteGraphClient(**vcfg["graph"])
...

This form cannot work if we don't wrap this into a try...except ModuleNotFound... stanza when the dep is actually not installed.

ardumont updated this revision to Diff 21320.Jun 28 2021, 5:40 PM

Fix build

swh-public-ci added a comment.Jun 28 2021, 5:42 PM

Build is green

Patch application report for D5935 (id=21320)

Rebasing onto 0bec623156...

Current branch diff-target is up to date.
Changes applied before test
commit eaab208f2242747f1821d361f6cf9625bb938fb4
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jun 28 15:36:26 2021 +0200

    Make swh.graph dependency optional
    
    It's not packaged nor ready yet as some internal reworking has been discussed.
    
    Related to T3412

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

Harbormaster completed remote builds in B22301: Diff 21320.Jun 28 2021, 5:43 PM
vlorentz accepted this revision.Jun 29 2021, 9:55 AM
This revision is now accepted and ready to land.Jun 29 2021, 9:55 AM
ardumont updated this revision to Diff 21322.Jun 29 2021, 10:03 AM

Drop spurious line in mypy

swh-public-ci added a comment.Jun 29 2021, 10:05 AM

Build is green

Patch application report for D5935 (id=21322)

Rebasing onto 0bec623156...

Current branch diff-target is up to date.
Changes applied before test
commit 9d0df34d412a235df1895e4d0077827371935ad8
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Mon Jun 28 15:36:26 2021 +0200

    Make swh.graph dependency optional
    
    It's not packaged nor ready yet as some internal reworking has been discussed.
    
    Related to T3412

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

Harbormaster completed remote builds in B22303: Diff 21322.Jun 29 2021, 10:05 AM
Closed by commit rDVAU9d0df34d412a: Make swh.graph dependency optional (authored by ardumont). · Explain WhyJun 29 2021, 10:11 AM
This revision was automatically updated to reflect the committed changes.
ardumont added a commit: rDVAU9d0df34d412a: Make swh.graph dependency optional.
ardumont mentioned this in T3412: Deploy vault "git bare" tarball cooker (swh-vault v0.6).Jun 29 2021, 10:16 AM
ardumont mentioned this in D5938: Make swh.graph dependency optional 2/2.Jun 29 2021, 11:00 AM
vlorentz added a task: T843: Vault: Add a "git bare" tarball cooker.Aug 2 2021, 1:56 PM

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
1 line--
1 line--
1 line--
5 lines--
swh/
vault/
14 lines76%
Loading...
cookers/
16 lines90%
Loading...
4 lines--

Diff 21320

View Options

mypy.ini

Loading...
View Options

requirements-swh-graph.txt

Loading...
View Options

requirements-swh.txt

Loading...
View Options

setup.py

Loading...
View Options

swh/vault/cli.py

Loading...
View Options

swh/vault/cookers/__init__.py

Loading...
View Options

tox.ini

Loading...
About · Developer information · Legal