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

Enhance local setup/launch with python script #2534

Merged
merged 21 commits into from
Mar 5, 2024

Conversation

Traf333
Copy link
Contributor

@Traf333 Traf333 commented Feb 28, 2024

Context

This PR introduces next changes:

  • moves local-setup folder to the root folder
  • adjusts launch.py
    • to consider multi worker setup with correct offset
    • omits use of --config option, as hardcoded json files are too specific and some of them don't work as expected

Copy link

linear bot commented Feb 28, 2024

docker/pnpm-lock.yaml Outdated Show resolved Hide resolved
@Kailai-Wang
Copy link
Collaborator

Cool attempt 👍 , can we keep both?
For some people (like me) they know exactly what they want and are used to one-liner

Maybe have a command line option -i for interactive mode

@Traf333
Copy link
Contributor Author

Traf333 commented Feb 28, 2024

Cool attempt 👍 , can we keep both? For some people (like me) they know exactly what they want and are used to one-liner

Maybe have a command line option -i for interactive mode

@Kailai-Wang by oneliner you want to select specific json config or do you mean to have ability to pass arguments with ports and stuff?
Or keep default preset, like Parachain in standalone mode + 1 worker?

Asking because I've noticed that hardcoded json configs don't work stable and some of them already outdated. It sounds reasonable to have -i though 👍

@Traf333
Copy link
Contributor Author

Traf333 commented Feb 28, 2024

By the way, if we are sticking to this approach, I would also like to include:

  1. getting rid of the bitacross-worker/local-setup and other python scripts
  2. removing *worker.json config files as they are static and might not working well, when somebody else run infra on the server
  3. updated Readme
  4. moved local-setup folder to the root directory

@BillyWooo
Copy link
Collaborator

"removing worker.json ", what's the replacement? Besides, are we still using python files?

Copy link
Member

@felixfaisal felixfaisal left a comment

Choose a reason for hiding this comment

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

Thanks @Traf333 , I think I'm a little bit biased, Here's what I think:

  1. Majority of the changes felt like the same logic in python ported to js. I don't see the benefits of moving from python to a js script. I may have missed the discussion on this.
  2. No update in Readme, Missing steps of installation such as yarn install etc.
  3. I'm of the same opinion, that I generally prefer commands and flags over an interactive script, it's quicker to restart and make changes. So would be great if we can have both.

@0xverin
Copy link
Contributor

0xverin commented Feb 29, 2024

I'm of the same opinion, that I generally prefer commands and flags over an interactive script, it's quicker to restart and make changes. So would be great if we can have both.

I agree with this opinion. :p

@Traf333 Traf333 changed the title Propose new interactive script for running dev env Enhance local setup/launch with python script Mar 1, 2024
local-setup/.env Outdated Show resolved Hide resolved
Copy link
Member

@felixfaisal felixfaisal left a comment

Choose a reason for hiding this comment

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

Thanks @Traf333 For revamping the entire PR
I tested the following:

  1. local-binary
  2. local-binary-standalone
  3. local-binary with two workers (WOW!)

It looks good from my side 🙌

Copy link
Contributor

@0xverin 0xverin left a comment

Choose a reason for hiding this comment

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

Thanks. I tested:

  • local binary

  • local-binary-standalone

LGTM!

@BillyWooo
Copy link
Collaborator

Is the "remote" working mode still valid?

@Traf333
Copy link
Contributor Author

Traf333 commented Mar 5, 2024

Is the "remote" working mode still valid?

@BillyWooo yes it works as well, but there are couple of moments we should be aware of.

  1. parachain should be run on the same machine
  2. offset for remote should also be considered. if you run command with offset 44 then your parachain port should be 9988
  3. without offset everything will work as before

if the remote mode would gain more popularity, in that case we can introduce extra argument for pointing to remote url

@Traf333 Traf333 merged commit 5534947 into dev Mar 5, 2024
31 checks passed
@Traf333 Traf333 deleted the p-375-propose-new-interactive-script-for-running-infra branch March 5, 2024 09:10
@Traf333
Copy link
Contributor Author

Traf333 commented Mar 5, 2024

Is the "remote" working mode still valid?

@BillyWooo yes it works as well, but there are couple of moments we should be aware of.

  1. parachain should be run on the same machine
  2. offset for remote should also be considered. if you run command with offset 44 then your parachain port should be 9988
  3. without offset everything will work as before

if the remote mode would gain more popularity, in that case we can introduce extra argument for pointing to remote url

it seems I was wrong. We still shifting ports if 9944 is busy, :( so for now remote option is unavaliable. I'll prepare fix with adding extra argument for remote url

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.

None yet

6 participants