Fix dependency resolving by ShadowJonathan · Pull Request #8367 · matrix-org/synapse

ShadowJonathan · 2020-09-21T15:22:21Z

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
Pull request includes a sign off
Code style is correct (run the linters)

Signed-off-by: Jonathan de Jong <jonathan@automatia.nl>

clokep · 2020-09-21T17:06:14Z

synapse/python_dependencies.py


    sys.stdout.writelines(req + "\n" for req in list_requirements())
+
+    check_requirements(dev=True)


Does something run this module at some point? When is this used?

No, currently nothing runs the module at this point, but this way, python -m synapse.python_dependencies can be suggested by someone from the synapse team to be ran by developers to check if their dependencies are alright, or if one of them is the wrong version.

clokep · 2020-09-21T17:08:15Z

changelog.d/8367.misc

@@ -0,0 +1 @@
+Fixes the way synapse distinguishes between runtime and developer dependencies.


We should use the same newsfragment as the PR that regressed this so that they'll show up together in the changelog.

clokep · 2020-09-21T17:08:57Z

synapse/python_dependencies.py


    if for_feature:
-        reqs = CONDITIONAL_REQUIREMENTS[for_feature]
+        reqs = CONDITIONAL_REQUIREMENTS.get(for_feature, [])


Why this change? It seems unrelated?

if for_feature is not a key in CONDITIONAL_REQUIREMENTS, it'll raise a KeyError, i think the right logic then is to return a default empty list

This is so that when check_requirements is called with a dependency list that does not exist anymore, any time in the future, it should not return a cryptic error, should i revert this?

richvdh

generally this feels overengineered to me. I don't really see why we need to complicate python_dependencies with this stuff rather than just putting the dev dependencies in setup.py.

ShadowJonathan · 2020-09-22T14:20:20Z

@richvdh it's because i feel this fixes some other problems besides #8365

richvdh · 2020-09-22T15:23:00Z

@richvdh it's because i feel this fixes some other problems besides #8365

well, perhaps you could clarify what they are. I feel like we've discussed this, asked you to take a different approach, and now you've ignored our requests and presented us with your own implementation without explaining how it works and why it is superior.

ShadowJonathan · 2020-09-22T16:09:33Z

synapse/python_dependencies.py

+# We exclude lint and test as those are developer requirements
+ALL_RUNTIME_REQUIREMENT_COMPONENTS = set(CONDITIONAL_REQUIREMENTS.keys()) - {
+    "lint",
+    "test",
+}
+


@richvdh This change filters out all developer dependencies from all conditional dependencies, by only excluding lint and test, and then flattening the resulting dependencies into a single set.

I could change this to be only a set with {"lint", "test"}, as this piece is only used in one part later on.

ShadowJonathan · 2020-09-22T16:12:16Z

synapse/python_dependencies.py

+    if for_feature is None:
+        # Check if the optional runtime dependencies are up to date. We allow them to not be
        # installed.
-        OPTS = sum(CONDITIONAL_REQUIREMENTS.values(), [])  # type: List[str]
+        OPTS = set().union(
+            *(
+                set(reqs)
+                for key, reqs in CONDITIONAL_REQUIREMENTS.items()
+                if key in ALL_RUNTIME_REQUIREMENT_COMPONENTS or dev
+            )
+        )  # type: Set[str]


@richvdh this is two compounded changes;

Change OPTS to be a flattened set, to deduplicate any elements, and have potential faster iteration.

if dev == False, only check default (runtime) dependencies, if dev == True, also check development dependencies, this is useful as an addition in __main__ down below, which does dev=True, making an invocation of the file (python -m synapse.python_dependencies) automatically also check those development dependencies.

ShadowJonathan · 2020-09-22T16:14:26Z

@richvdh it's because i feel this fixes some other problems besides #8365

well, perhaps you could clarify what they are. I feel like we've discussed this, asked you to take a different approach, and now you've ignored our requests and presented us with your own implementation without explaining how it works and why it is superior.

I left some comments doing exactly that last part, sorry for my abrasive changes, I thought this was a rational change overall.

ShadowJonathan · 2020-09-22T16:23:06Z

Some extra context, setup.py does

synapse/setup.py

Lines 94 to 95 in b29a9bd

    
           # Make `pip install matrix-synapse[all]` install all the optional dependencies. 
        
           CONDITIONAL_REQUIREMENTS["all"] = list(ALL_OPTIONAL_REQUIREMENTS)

synapse/setup.py

Lines 103 to 104 in b29a9bd

    
           install_requires=REQUIREMENTS, 
        
           extras_require=CONDITIONAL_REQUIREMENTS,

Adding the following to it would split the concern of "dependency notation" between synapse/python_dependencies.py and setup.py, i think adding this to setup.py when nearly everything is already being defined in synapse.python_dependencies would lead to some (more) loss of oversight in the matter.

# Make `pip install matrix-synapse[all]` install all the optional dependencies.
CONDITIONAL_REQUIREMENTS["all"] = list(ALL_OPTIONAL_REQUIREMENTS)

CONDITIONAL_REQUIREMENTS["lint"] = ["isort==5.0.3", "black==19.10b0", "flake8-comprehensions", "flake8"]

The synapse codebase wants to define dependencies in synapse.python_dependencies, that's fine, but i think then that invisible agreement should not be breached by adding extra (dependent (because of tox installing linting packages from it)) behaviour in a different spot, effectively having to have someone look up in multiple places if some kind of bug was present here.

richvdh

ok so to summarise, I think you are saying you want to put it in python_dependencies because that is where existing dependencies are listed and you don't want to split them up.

my counterpoint is: this complicates python_dependencies, making it harder to understand. Additionally I would say that python_dependencies is explicitly for runtime dependencies, and having other dependencies elsewhere is reasonable.

I don't particularly want to debate this any further. If you'd like to update this PR to change setup.py instead, that would be great. Otherwise I can revert the PR that caused the regression. Thank you.

clokep · 2020-09-22T18:22:59Z

I think that #8377 is what @richvdh is asking for.

ShadowJonathan added 2 commits September 21, 2020 17:19

Fix dependency resolving

3187d2f

Add new snippet, make linter happy

4c9b543

clokep suggested changes Sep 21, 2020

View reviewed changes

richvdh reviewed Sep 21, 2020

View reviewed changes

ShadowJonathan commented Sep 22, 2020

View reviewed changes

richvdh suggested changes Sep 22, 2020

View reviewed changes

clokep mentioned this pull request Sep 22, 2020

Do not check lint/test dependencies at runtime. #8377

Merged

richvdh closed this in #8377 Sep 23, 2020


		sys.stdout.writelines(req + "\n" for req in list_requirements())

		check_requirements(dev=True)

		@@ -0,0 +1 @@
		Fixes the way synapse distinguishes between runtime and developer dependencies. No newline at end of file

Uh oh!

Conversation

ShadowJonathan commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Uh oh!

clokep Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

ShadowJonathan Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

clokep Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

clokep Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

ShadowJonathan Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

ShadowJonathan commented Sep 22, 2020

Uh oh!

richvdh commented Sep 22, 2020

Uh oh!

ShadowJonathan Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShadowJonathan Sep 22, 2020

Choose a reason for hiding this comment

Uh oh!

ShadowJonathan commented Sep 22, 2020

Uh oh!

ShadowJonathan commented Sep 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

clokep commented Sep 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ShadowJonathan commented Sep 21, 2020 •

edited

Loading

ShadowJonathan Sep 22, 2020 •

edited

Loading

ShadowJonathan commented Sep 22, 2020 •

edited

Loading