Page MenuHomeSoftware Heritage

Make swh.graph dependency optional
ClosedPublic

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

Details

Summary

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

Related to T3412

Test Plan

tox

Diff Detail

Repository
rDVAU Software Heritage Vault
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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.

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

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.

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

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

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!?

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.

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

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

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.

Adapt according to discussions

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

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.

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.

This revision is now accepted and ready to land.Jun 29 2021, 9:55 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.

This revision was automatically updated to reflect the committed changes.