paritytech / bench-bot

ISC License
9 stars 19 forks source link

Support for any repo + remote exec #10

Open enthusiastmartin opened 3 years ago

enthusiastmartin commented 3 years ago

Hi,

In HydraDX, we liked the bench-bot and wanted to use it in similar way it is done in substrate/polkadot repository.

However , it needed some adjustments so it could be used with our Substrate project.

And instead of just hacking the bench-bot to fit our project - we thought to make changes in a way that the bench-bot can be used with any other substrate project which follows standard Substrate template node repository.

It might be beneficial for other substrate projects.

So within this PR, we added handling of other than substrate and polkadot repositories.

Currently, it is all done separately to keep the benchmarking process for substrate/polkadot intact as it is slightly differs in few things. And it should fit any project which uses substrate template node as a template.

It is actually still work in progress ( hence the draft pr) - I am still adding few improvements.

And I also want to add some features like whitelisted users and cancel benchmarks - which I saw too in Bench-bot v2 spec created by @shawntabrizi ( but that might be done separately ).

But i thought why not just to create PR to propose and ask you for opinions.

We also needed to run the benchmarks on specific machine , so there is also implemented a possibility to run all commands on a remote machine by specifying remote host/user.

shawntabrizi commented 3 years ago

@enthusiastmartin hey, i just saw this for the first time.

Is this something you are continuing to work on?

Does your changes work as is?

enthusiastmartin commented 3 years ago

@enthusiastmartin hey, i just saw this for the first time.

Is this something you are continuing to work on?

Does your changes work as is?

Yes, as this PR is - it works.

We made few other modifications for our purposes, but that's just our needs.

I have a plan to add some features which I had mentioned ( mainly the whitelist of users and cancel ) soon. So yes, still planning to continue some work on this.

Would you be happy with such features as well ?

shawntabrizi commented 3 years ago

@enthusiastmartin sorry for delayed responses here.

I am very happy to merge any of these additional features in. Please let me know when it is ready to review and I will bring it in!

cla-bot-2021[bot] commented 3 years ago

User @enthusiastmartin, please sign the CLA here.

joao-paulo-parity commented 3 years ago

Hi @enthusiastmartin, I'll be taking over maintenance for this bot from now on

I saw that you split the modules and reorganized some of the code as part of your changes. I don't like the idea of building upon the existing code as it currently is really hard to maintain and reason about. Some of your changes fit in the notion of a "refactor" which is what I plan to do soon, but not with the code base in its current state. So for a starting point I'll try to merge your PRs (#10 and #11) into a "develop" branch and start reworking from there.

Once this rework is done I'll reference your PRs in the new PR I'll be opening.

enthusiastmartin commented 3 years ago

Hi @enthusiastmartin, I'll be taking over maintenance for this bot from now on

I saw that you split the modules and reorganized some of the code as part of your changes. I don't like the idea of building upon the existing code as it currently is really hard to maintain and reason about. Some of your changes fit in the notion of a "refactor" which is what I plan to do soon, but not with the code base in its current state. So for a starting point I'll try to merge your PRs (#10 and #11) into a "develop" branch and start reworking from there.

Once this rework is done I'll reference your PRs in the new PR I'll be opening.

Ok, great. fine with with me of course.

yes, i kind of agree. i added this work on top of what was currently there but i did not want to touch and change the implementation for substtrate/polkadot repo as mentioned in my PR description.