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.

Sign up for GitHub

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

Jump to bottom

Implement persistent, local reputation + integration with the external Reputation System #214

Merged
merged 30 commits into from
Mar 28, 2024
Merged

Implement persistent, local reputation + integration with the external Reputation System #214

merged 30 commits into from
Mar 28, 2024

Conversation

shadeofblue
Copy link
Contributor

@shadeofblue shadeofblue commented Mar 14, 2024 •
edited
Loading

  • Add Tortoise ORM + Aerich migration system
  • Add FactoryBoy to facilitate easy creation of complex mock data structures
  • Reputation schema, models + seed data, based on the previously-hardcoded values
  • Add an adminstrative CLI tool to manage aerich migrations for the reputation db
  • Add CLI to the new reputation module to manage and display the reputation data
    • add ability to update the local reputation from the remote Reputation System
  • Port provider blacklist plugin to the new reputation
  • Port provider scorer plugin to the new, reputation-system-based reputation
  • Automatically update the reputation from the external source on webserver start
  • Add factories for Demand and Proposal resources
  • Add a unit tests to the provider blacklist
  • Add a unit test to the reputation scorer

shadeofblue and others added 14 commits March 7, 2024 17:07
@shadeofblue
initial reputation db
4b020c8
@shadeofblue
add reputation Network model
9154e9a 
fix initialization
@shadeofblue
reputation db initial migrations
4e435bf
@shadeofblue
reputation managent commands
a23b943
@shadeofblue
remote reputation system updater
ead071c
@shadeofblue
populate reputation data from the reputation system
af66725
@shadeofblue
fix reputation list display, add progressbar
4437810
@shadeofblue
formatting
1f04742
@shadeofblue
* add local reputation blacklist plugin
11bde3f 
* add blacklist queryset methods to `NodeReputation`
* add `with_network` decorator to optimise reputation system's cli commands
* add reputation system context to the webserver
* use the local reputation blacklist in `network_stats`
@shadeofblue
* cleanup the hardcoded providers' blacklist
bfc9a5c 
* optimize the reputation list query
@shadeofblue
lint/format
dbb24e9
@shadeofblue
scoring plugin ... in progress
1dcadf6
@shadeofblue
add reputation-based scoring
3f9607b
@shadeofblue
move reputation to the main module
feedd22
@shadeofblue shadeofblue force-pushed the blue/reputation branch 4 times, most recently from fdb26c4 to 076a719 Compare March 19, 2024 19:17
@shadeofblue
factoryboy + blacklist unit test
7249c02 
* introducing factoryboy
+ Proposal/Demand factories
+ blacklist unit tests
@shadeofblue shadeofblue marked this pull request as ready for review March 22, 2024 16:49
@shadeofblue shadeofblue changed the title Blue/reputation Implement persistent, local reputation + integration with the external Reputation System Mar 22, 2024
lucekdudek
lucekdudek reviewed Mar 25, 2024
View reviewed changes
Copy link
Contributor

@lucekdudek lucekdudek left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Most of it looks very good, I added few comments and I am curious what you think about them.

ray_on_golem/cli.py Show resolved Hide resolved
ray_on_golem/reputation/plugins/score.py Outdated Show resolved Hide resolved
ray_on_golem/reputation/plugins/score.py Show resolved Hide resolved
ray_on_golem/reputation/updater.py Show resolved Hide resolved
ray_on_golem/reputation/updater.py Show resolved Hide resolved
ray_on_golem/reputation/updater.py Outdated Show resolved Hide resolved
ray_on_golem/reputation/updater.py Outdated Show resolved Hide resolved
@shadeofblue
Copy link
Contributor Author

@lucekdudek updated the code with the suggestions that I could :)

lucekdudek
lucekdudek approved these changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@lucekdudek lucekdudek left a comment

Choose a reason for hiding this comment

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

Approved, please consider changing that argument name.

approxit
approxit reviewed Mar 27, 2024
View reviewed changes
Copy link
Contributor

@approxit approxit left a comment

Choose a reason for hiding this comment

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

I'm a fan of weights on stuff that comes from external reputation, neat!

Also, a handful of code-style and asyncio-related stuff.

ray_on_golem/reputation/plugins/score.py Outdated Show resolved Hide resolved
ray_on_golem/reputation/plugins/score.py Outdated Show resolved Hide resolved
ray_on_golem/reputation/service.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

More typing on methods would be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly think it wouldn't make much difference in case of this particular class... especially that the original methods it uses are not typed either...

ray_on_golem/reputation/updater.py Outdated Show resolved Hide resolved
ray_on_golem/reputation/updater.py Outdated Show resolved Hide resolved
ray_on_golem/reputation/updater.py Outdated Show resolved Hide resolved
ray_on_golem/server/main.py Outdated Show resolved Hide resolved
ray_on_golem/server/main.py Outdated Show resolved Hide resolved
ray_on_golem/server/services/golem/golem.py Outdated Show resolved Hide resolved
tests/unit/reputation/test_plugins_score.py Outdated Show resolved Hide resolved
@shadeofblue
Copy link
Contributor Author

@approxit updated the pull request with your suggestions

approxit
approxit reviewed Mar 28, 2024
View reviewed changes
ray_on_golem/server/main.py Outdated
@@ -171,14 +171,14 @@ async def reputation_service_ctx(app: web.Application) -> None:
updaters = [
ReputationUpdater(network) for network in ["polygon", "mumbai", "goerli", "holesky"]
]
update_tasks = [asyncio.create_task(updater.update()) for updater in updaters]
update_tasks = [
create_task_with_logging(updater.update(), trace_id="reputation_service_ctx")
Copy link
Contributor

Choose a reason for hiding this comment

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

The trick with trace_id works when its values are unique. In that way, logs can differentiate task instances from each other. Refer to its usecases with get_trace_id_name.

ray_on_golem/reputation/updater.py Outdated
def reputation_uri(self):
return f"{REPUTATION_SYSTEM_URI}{REPUTATION_SYSTEM_PROVIDER_SCORES}?network={self._network}"
self.reputation_uri = (
URL(REPUTATION_SYSTEM_URI)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of parsing yarl.URL here, let's just keep it as a value of constant instead - as we do for many other constants - rich object instead of raw naive value.

@shadeofblue shadeofblue merged commit 718e6a2 into develop Mar 28, 2024
2 checks passed
@shadeofblue shadeofblue deleted the blue/reputation branch March 28, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@approxit approxit approxit approved these changes

@lucekdudek lucekdudek lucekdudek approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shadeofblue @approxit @lucekdudek