Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

try-runtime-cli: improve ci stability #14030

Merged
merged 16 commits into from
Apr 27, 2023

Conversation

liamaharon
Copy link
Contributor

@liamaharon liamaharon commented Apr 27, 2023

  • Updates HttpClientBuilder config to fix some stability issues with the CI and increase the max possible batch size
  • Limits parallelism to 4 threads. Performance improvement beyond 4 is negligible, and can lead to some weird stability issues with the HttpClient ("dispatch dropped without returning error" when using Client from tests hyperium/hyper#2112)
    • Ideally we would refactor the code to remove all threading and rely on tokio futures for request parallelism (or use just two threads: one for inserting keys and one dedicated for http requests using tokio futures). However limiting the number of threads seems sufficient for fixing the CI issue for now. Larger refactor will come later, probably when we implement lazy-download.
  • CI tested against this branch here: [DNM] test try-runtime ci stability polkadot#7133
    • westend still failing, not due to download but due to an on-runtime-upgrade try-runtime hook actually failing: the on_chain version is equal or more than the current one. I guess we need to update its Migrations struct? If so, I'm happy to handle that in a seperate PR.

Closes #14013 and #14006

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Looks good to me

It would be safer to create a new HttpClient in each thread spawned in the "remote externalities" than to limit the number of threads, fine for now

@liamaharon liamaharon added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Apr 27, 2023
@liamaharon
Copy link
Contributor Author

Looks good to me

It would be safer to create a new HttpClient in each thread spawned in the "remote externalities" than to limit the number of threads, fine for now

Yup I was considering this but it seems like it would require a larger refactor. We'll be doing a large refactor later anyway for lazy-download, and CI seems happy with this configuration, so decided not to do it here.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Thanks!

utils/frame/remote-externalities/src/lib.rs Show resolved Hide resolved
utils/frame/remote-externalities/src/lib.rs Show resolved Hide resolved
@liamaharon liamaharon merged commit 5d1b74c into master Apr 27, 2023
@liamaharon liamaharon deleted the liam-try-runtime-ci-stability-improvements branch April 27, 2023 14:03
gpestana pushed a commit that referenced this pull request May 4, 2023
* hardcode 1 thread

* ci stability

* fix comment

* improve comment

* kick ci

* kick ci

* update expect message

* improve comment

* kick ci

* kick ci

* configure threads with env var

* fix threads env var

* fix threads env var
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* hardcode 1 thread

* ci stability

* fix comment

* improve comment

* kick ci

* kick ci

* update expect message

* improve comment

* kick ci

* kick ci

* configure threads with env var

* fix threads env var

* fix threads env var
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

try-runtime-cli error Invalid input: Batch failed
3 participants