Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP Add dependency-injector to Tribler #6197

Closed
wants to merge 4 commits into from

Conversation

drew2a
Copy link
Contributor

@drew2a drew2a commented Jul 1, 2021

This is an example of DI in Tribler.

I've tested it on initial_filling.py and run_tribler.py.

What can I say after a day of work: it works but the framework is surprisingly difficult to understand.
I spent all day trying to make this example runnable.

Partially it is because of the session.py organization, but mainly because of the unintuitive framework.
(I am not stating that it is bad, but only that it is unintuitive).

Please disregard the writing style, I didn't work on it.
I want to show you roughly what the Tribler with DI will look like.

Framework: https://python-dependency-injector.ets-labs.org/
@ichorid example: https://github.com/Tribler/tribler/pull/6185/files

References:
#4953
#6177

working_dir, config_path)

self._interval_in_sec = interval_in_sec
self._output_file_path = output_file_path

self.application = containers.ApplicationContainer(state_dir=config.state_dir)
self.application.config.from_pydantic(config)
self.application.wire(modules=[Session])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good start point to discover PR.

endpoint = providers.Singleton(
DispatcherEndpoint,
providers.List(providers.Object("UDPIPv4")),
UDPIPv4=providers.Dict(port=providers.Factory(config.port), ip=providers.Factory(config.address)),
Copy link
Contributor

@ichorid ichorid Jul 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this line looks not too intuitive 😕. I would expect it to be just UDPIPv4={'port':config.port}, etc. Why the Factory stuff is necessary here, what's the semantic reason? Lazy initialization?

Copy link
Contributor Author

@drew2a drew2a Jul 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be replaced by:

    endpoint = providers.Singleton(
        DispatcherEndpoint, ["UDPIPv4"],
        UDPIPv4=providers.Dict(port=config.port, ip=config.address),
    )

Explanations later...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use UDPIPv4={'port':config.port} you'll get:

{'UDPIPv4': {'port': <dependency_injector.providers.ConfigurationOption('config.port') at 0x112961eb0>}}

from tribler_core.utilities.path_util import Path


class TrustChainKeys:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, it is better to leave only the single keypair property in the code, loading either the real key or the testnet key based on the config option. AFAIK, there are no situations where both keys are used in the code simultaneously.

@ichorid
Copy link
Contributor

ichorid commented Jul 1, 2021

Looks promising! I see you're going to introduce DependencyInjector, but you're still keeping the Session in, right?

@qstokkink
Copy link
Contributor

Looks good 👍 All my implementation feedback is already covered by @ichorid. I have two meta questions about what you wrote in O.P.:

What can I say after a day of work: it works but the framework is surprisingly difficult to understand.
I spent all day trying to make this example runnable.

  1. Did you enjoy working with DI (is it intuitive, verbose, etc.)? In other words, did you feel like you were fighting the framework or did you feel like the framework was helping you?
  2. Do you think DI is prohibitively difficult for new developers? It's not a good thing if this scares off new or external developers.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 2, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@drew2a drew2a closed this Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants