-
Notifications
You must be signed in to change notification settings - Fork 632
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
feat: swarmable FT loadtest #9111
Conversation
To make the locust-based loadtest swarmable: - Create N accounts with deterministic name and keys - N workers pick one each to fund their users Note that the naive approach to share the same funding account across workers doesn't work. They will constantly invalidate each others transactions due to nonces. This commit also moves some code out of `locustfile.py` to `base.py` and `ft.py` for a better separation of concerns. And it includes small fixes: - refresh_nonce sometimes raises a `KeyError` when the account was just created, catch it crudely in a try-catch retry loop - Provide `locust_name` arguments to show better statistics on requests
# TODO: Create accounts in parallel | ||
for i in range(num_ft_contracts): | ||
# Prefix that makes accounts unique across workers | ||
prefix = str(hash(str(worker_id) + str(i)))[-6:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to take a hash here and can't just use a simpler f"{worker_id}_{i}"
which also should be unique?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shuffling with a hash avoids locality in the state trie.
I will add this as a comment.
Also, we should be more careful with the hash, since this always produced a number and we want to have all valid account id characters in the output in order to cover all shards. The same is (even more) true for account generation in NearUser
. I'll add TODOs to the code but don't want to fix it as part of this PR.
# CLI args | ||
funding_account = None | ||
# every worker needs a funding account to create its users, eagerly create them in the master | ||
if isinstance(environment.runner, runners.MasterRunner): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A modern (Python >=3.10) way to match the class of the object would be to use pattern matching [1]:
match environment.runner:
case runners.MasterRunner:
# Handle MasterRunner case.
case runners.WorkerRunner:
# Handle WorkerRunner case.
...
case other_runner:
# Handle all other cases.
[1] https://peps.python.org/pep-0636/#adding-a-ui-matching-objects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that looks much nicer.
I don't know our official policy regarding python versions but I try to be on the conservative side to ensure compatibility with preinstalled python on older OSs and docker images. But I could be convinced otherwise quite easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably optimize for developer productivity here, as we expect this code to be mainly used by devs on machines that they control. In this case, the version should be easy to install on common OSes.
Ubuntu 22.04 is on Python 3.10, Archlinux is on Python 3.11, so 3.10 sounds like a safe bet.
Our Docker setup uses Ubuntu 18.04, but its support is ending in June 2023 [1] and we'll need to migrate to new version that will be >= 20.04 soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points. Well, I know I am still running private Ubuntu 20.04 systems (python 3.8 by default) but I just noticed the official support for it was dropped a month ago.^^
Anyway, I opened #9125
- refactor duplication in key.py - use `getattr` - remove superfluous variables - add comments to justify hash prefix in account ids - use retrying library instead of manual retry-loop
I didn't test near#9111 it properly, this doesn't actually work as expected. We need `retrying` not `retry`.
I didn't test #9111 properly, this doesn't actually work as expected. We need `retrying` not `retry`.
To make the locust-based loadtest swarmable: - Create N accounts with deterministic name and keys - N workers pick one each to fund their users Note that the naive approach to share the same funding account across workers doesn't work. They will constantly invalidate each others transactions due to nonces. This commit also moves some code out of `locustfile.py` to `base.py` and `ft.py` for a better separation of concerns. And it includes small fixes/improvements: - allow multiple FT contracts per worker - refresh_nonce sometimes raises a `KeyError` when the account was just created, catch it crudely in a try-catch retry loop - Provide `locust_name` arguments to show better statistics on requests
I didn't test #9111 properly, this doesn't actually work as expected. We need `retrying` not `retry`.
To make the locust-based loadtest swarmable: - Create N accounts with deterministic name and keys - N workers pick one each to fund their users Note that the naive approach to share the same funding account across workers doesn't work. They will constantly invalidate each others transactions due to nonces. This commit also moves some code out of `locustfile.py` to `base.py` and `ft.py` for a better separation of concerns. And it includes small fixes/improvements: - allow multiple FT contracts per worker - refresh_nonce sometimes raises a `KeyError` when the account was just created, catch it crudely in a try-catch retry loop - Provide `locust_name` arguments to show better statistics on requests
I didn't test near#9111 properly, this doesn't actually work as expected. We need `retrying` not `retry`.
To make the locust-based loadtest swarmable: - Create N accounts with deterministic name and keys - N workers pick one each to fund their users Note that the naive approach to share the same funding account across workers doesn't work. They will constantly invalidate each others transactions due to nonces. This commit also moves some code out of `locustfile.py` to `base.py` and `ft.py` for a better separation of concerns. And it includes small fixes/improvements: - allow multiple FT contracts per worker - refresh_nonce sometimes raises a `KeyError` when the account was just created, catch it crudely in a try-catch retry loop - Provide `locust_name` arguments to show better statistics on requests
To make the locust-based loadtest swarmable: - Create N accounts with deterministic name and keys - N workers pick one each to fund their users Note that the naive approach to share the same funding account across workers doesn't work. They will constantly invalidate each others transactions due to nonces. This commit also moves some code out of `locustfile.py` to `base.py` and `ft.py` for a better separation of concerns. And it includes small fixes/improvements: - allow multiple FT contracts per worker - refresh_nonce sometimes raises a `KeyError` when the account was just created, catch it crudely in a try-catch retry loop - Provide `locust_name` arguments to show better statistics on requests
I didn't test #9111 properly, this doesn't actually work as expected. We need `retrying` not `retry`.
To make the locust-based loadtest swarmable:
Note that the naive approach to share the same funding account across workers doesn't work. They will constantly invalidate each others transactions due to nonces.
This commit also moves some code out of
locustfile.py
tobase.py
andft.py
for a better separation of concerns.And it includes small fixes/improvements:
KeyError
when the account was just created, catch it crudely in a try-catch retry looplocust_name
arguments to show better statistics on requests