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

[ft-benchmark] some fixes for benchmark infra #11604

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

MCJOHN974
Copy link
Contributor

With this PR we will be finally able to run benchmark and send data to DB fully automatically

@MCJOHN974 MCJOHN974 requested a review from aborg-dev June 18, 2024 17:04
@MCJOHN974 MCJOHN974 requested a review from a team as a code owner June 18, 2024 17:04
@MCJOHN974
Copy link
Contributor Author

Whoops, we actually able to change some hardcoded values in ft-benchmark-data-sender.py to command line arguments. Will fix it before landing

Copy link

codecov bot commented Jun 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.48%. Comparing base (442fbfc) to head (b7f1ab2).
Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11604      +/-   ##
==========================================
- Coverage   71.52%   71.48%   -0.05%     
==========================================
  Files         786      788       +2     
  Lines      160573   160774     +201     
  Branches   160573   160774     +201     
==========================================
+ Hits       114849   114923      +74     
- Misses      40710    40836     +126     
- Partials     5014     5015       +1     
Flag Coverage Δ
backward-compatibility 0.23% <ø> (-0.01%) ⬇️
db-migration 0.23% <ø> (-0.01%) ⬇️
genesis-check 1.36% <ø> (-0.01%) ⬇️
integration-tests 37.71% <ø> (+<0.01%) ⬆️
linux 68.90% <ø> (-0.04%) ⬇️
linux-nightly 70.97% <ø> (-0.05%) ⬇️
macos 52.61% <ø> (+0.06%) ⬆️
pytests 1.59% <ø> (-0.01%) ⬇️
sanity-checks 1.39% <ø> (-0.01%) ⬇️
unittests 66.14% <ø> (-0.07%) ⬇️
upgradability 0.28% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

subprocess.run(f"cargo run -p cli -- insert-ft-transfer {fp.name}",
shell=True)
fp.close()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should be able to remove flush and fsync by moving this close call after json.dump.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately no. Probably setting some options while creating tmp file object can help, but I spent a lot of time on this function and didn't find such options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error message was a bit unobvious, so I spent a time playing with tmp file options and place of fp.close(), before figured out, that it just don't flushing for some reason

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide steps to reproduce this? According to Python documentation, close is guaranteed to call flush: https://docs.python.org/3/library/io.html#io.IOBase.close

Copy link
Contributor

Choose a reason for hiding this comment

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

state_size = (int(
subprocess.check_output(["du", "-s", "~/.near/localnet/node0/data"],
stderr=subprocess.PIPE,
shell=True).decode("utf-8").split()[0]) * 1024)
processed_transactions = []
time_begin = datetime.now()
while True:
print(f"Data sender loop. Time elapsed: {(datetime.now() - time_begin).seconds} seconds")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use a logging module for this. It will automatically include the time in the log tine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably time from start of this loop is more informative than just time. But in general I added this just to see if this script started



# TODO: send signal to this process if ft-benchmark.sh decided to switch neard to another commit.
# add handling of this signal to this script
if __name__ == "__main__":

parser = argparse.ArgumentParser(description="Process some integers.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make description more descriptive? :)

@@ -52,9 +52,19 @@ def main() -> None:

args = parser.parse_args()

time_seconds = args.time
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can use some library for this? E.g. there is parse_timespan in Locust: https://github.com/locustio/locust/blob/1fc3143f1f275c6b311c9cecfab151410a76467e/locust/util/timespan.py#L5

@MCJOHN974 MCJOHN974 force-pushed the viktar/ft-benchmark-automatization branch from 4014886 to f317f2e Compare June 19, 2024 11:00
@MCJOHN974 MCJOHN974 force-pushed the viktar/ft-benchmark-automatization branch from f317f2e to b7f1ab2 Compare June 19, 2024 11:01
@MCJOHN974 MCJOHN974 enabled auto-merge June 19, 2024 11:02
@MCJOHN974 MCJOHN974 added this pull request to the merge queue Jun 19, 2024
Merged via the queue into master with commit 7106d8e Jun 19, 2024
29 of 30 checks passed
@MCJOHN974 MCJOHN974 deleted the viktar/ft-benchmark-automatization branch June 19, 2024 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants