Omni-Node renamings #5915

serban300 · 2024-10-03T12:04:08Z

moved the omni-node lib from
cumulus/polkadot-parachain/polkadot-parachain-lib to
cumulus/polkadot-omni-node/lib
renamed polkadot-parachain-lib to polkadot-omni-node-lib
added polkadot-omni-node binary

Related to #5566

skunert · 2024-10-04T07:10:12Z

I am not entirely sure if we gain something from this naming. To us its maybe obvious. But looking from the outside, polkadot-parachain or even just parachain-node sounds more intuitive and less surprising to me. In the end this is exactly what it is, just a parachain-node.

serban300 · 2024-10-04T07:46:44Z

I am not entirely sure if we gain something from this naming. To us its maybe obvious. But looking from the outside, polkadot-parachain or even just parachain-node sounds more intuitive and less surprising to me. In the end this is exactly what it is, just a parachain-node.

@kianenigma what do you think ?

kianenigma · 2024-10-04T09:38:35Z

(have not read the code yet)

I am not surprised to see pushback on the renaming 🙈

I am in favor of the renaming. Here's my reasons:

It amplifies the code-word of the project "omni-something". It is a guess, but I think it is more comprehensible to repeat the omni-prefix in all such tools.
Calling it parachain-node and/or polkadot-parachain prevents us from ever even imagining adding more abilities related to being a solo-chain to this node, something that I guess even parachain teams reasonably want esp. to run a testnet.
It changes nothing about polkadot-parachain: It remains as is, and it is indeed "Just a polkadot parachain node" as @skunert puts it.

Perhaps the confusion is partly because my original issue was lacking on point (remove any chainspec from the generic polkadot-omni-node). The outcome of this PR should be:

rename the library crate to polkadot-omni-node-lib. This is hidden from all users atm, because no one is using this crate yet (unless if you are @xlc and acala-node).
polkadot-parachain remains 100% as-is, and it is an instantiation of polkadot-omni-node-lib with all polkadot-related chainspecs hardcoded. This makes perfect sense to me in terms of naming: A parachain node, built from the generic polkadot-omni-node-lib, that is specialized for running Polkadot system chains, ergo polakdot-parachain.
polkadot-omni-node is a new binary, which is a raw instantiation of polkadot-omni-node-lib without any chainspecs or specialization.

So when looked at this way, we are not really touching polkadot-parachain. It remains as-is, and we are adding a new binary that is the more generic version of it.

kianenigma · 2024-10-04T09:41:09Z

Cargo.toml

-	"cumulus/polkadot-parachain",
-	"cumulus/polkadot-parachain/polkadot-parachain-lib",
+	"cumulus/polkadot-omni-node/polkadot-omni-node-lib",
+	"cumulus/polkadot-omni-node/polkadot-parachain",


So just looking at this file, i can confirm that this is not what I intended (as noted, my original issue was not super clear). High level, expect to see something like:

cumulus/polkadot-parachain should stay. This is also crucial for our internal devops to not put a bounty on my head.
cumulus/polkadot-omni-node should be a new crate.
cumulus/polkadot-parachain-lib -> cumulus/polkadot-omni-node/lib

Sorry, I misunderstood the requirement a bit. Will fix it.

skunert · 2024-10-04T09:49:32Z

(have not read the code yet)

I am not surprised to see pushback on the renaming 🙈

I am in favor of the renaming. Here's my reasons:

1. It amplifies the code-word of the project "omni-_something_". It is a guess, but I think it is more comprehensible to repeat the omni-prefix in all such tools.

2. Calling it `parachain-node` and/or `polkadot-parachain` prevents us from ever even _imagining_ adding more abilities related to being a solo-chain to this node, something that I [guess even parachain teams reasonably want](https://github.com/orgs/paritytech/projects/157/views/1?pane=issue&itemId=82036322) esp. to run a testnet.

3. It changes nothing about polkadot-parachain: It remains as is, and it is indeed "Just a polkadot parachain node" as @skunert puts it.

Perhaps the confusion is partly because my original issue was lacking on point (remove any chainspec from the generic polkadot-omni-node). The outcome of this PR should be:

0. rename the library crate to `polkadot-omni-node-lib`. This is hidden from all users atm, because no one is using this crate yet (unless if you are @xlc and `acala-node`).

1. `polkadot-parachain` remains 100% as-is, and it is an instantiation of `polkadot-omni-node-lib` with all polkadot-related chainspecs hardcoded. This makes perfect sense to me in terms of naming: A parachain node, built from the generic `polkadot-omni-node-lib`, that is specialized for running Polkadot system chains, ergo `polakdot-parachain`.

2. `polkadot-omni-node` is a new binary, which is a raw instantiation of `polkadot-omni-node-lib` without any chainspecs or specialization.

So when looked at this way, we are not really touching polkadot-parachain. It remains as-is, and we are adding a new binary that is the more generic version of it.

These are good points, I also forgot about frame-omni-bencher which goes nicely with this.

kianenigma · 2024-10-04T14:46:06Z

These are good points, I also forgot about frame-omni-bencher which goes nicely with this.

In my dreams, the binary list that one would see in our release pages would be:

polkadot-parachain
polkadot-omni-node
polkadot-omni-bencher
polkadot-omni-chain-spec-builder

Not to late to rename them? idk. But given https://github.com/orgs/paritytech/projects/157/views/1?pane=issue&itemId=78432998 in the roadmap, I won't push for this. Goal is to allow users to to all of this in just polkadot-omni-node.

@serban300 another small action item that comes to mind here is to ensure that as a consequence of this PR, polkadot-omni-node is also released. This requires a few GH/GL action files to be edited, #5387 is probably a good example. CC @EgorPopelyaev

- moved the omni-node lib from `cumulus/polkadot-parachain/polkadot-parachain-lib` to `cumulus/polkadot-omni-node/lib` - renamed `polkadot-parachain-lib` to `polkadot-omni-node-lib`

serban300 · 2024-10-07T13:32:33Z

So just looking at this file, i can confirm that this is not what I intended (as noted, my original issue was not super clear). High level, expect to see something like:

cumulus/polkadot-parachain should stay. This is also crucial for our internal devops to not put a bounty on my head. cumulus/polkadot-omni-node should be a new crate. cumulus/polkadot-parachain-lib -> cumulus/polkadot-omni-node/lib

@kianenigma @skunert I addressed this comment. I had to force push because there were a lot of conflicts. Can you PTAL ? This PR only contains the renamings and also added the polkadot-omni-node binary. I will implement the part about publishing the polkadot-omni-node binary in a different PR. Would like to merge this one as soon as possible to avoid merge conflicts since it moves a lot of files.

skunert

Nice!

skunert · 2024-10-08T08:58:43Z

prdoc/pr_5915.prdoc

+doc:
+  - audience: Node Dev
+    description: |
+      This PR renames the `polkadot-parachain-lib` crate to `polkadot-omni-node-lib`.


We could mention the introduction of polkadot-omni-node binary.

Good point ! Done.

cumulus/polkadot-omni-node/Cargo.toml

cumulus/polkadot-omni-node/src/main.rs

* master: (28 commits) `substrate-node`: removed excessive polkadot-sdk features (#5925) Rename QueueEvent::StartWork (#6015) [ci] Remove quick-benchmarks-omni from GitLab (#6014) Set larger timeout for cmd.yml (#6006) Fix `0003-beefy-and-mmr` test (#6003) Remove redundant XCMs from dry run's forwarded xcms (#5913) Add RadiumBlock bootnodes to Coretime Polkadot Chain spec (#5967) Bump strum from 0.26.2 to 0.26.3 (#5943) Add PVF execution priority (#4837) Snowbridge V2 docs (#5902) Fix u256 conversion in BABE (#5994) [ci] Move test-linux-stable-no-try-runtime to GHA (#5979) Bump PoV request timeout (#5924) [Release/CI] Github flow to build `polkadot`/`polkadot-parachain` rc binaries and deb package (#5963) [ci] Remove short-benchmarks from Gitlab (#5988) Disable flaky tests reported in 5972/5973/5974 (#5976) Bump some dependencies (#5886) bump zombienet version and set request for k8s (#5968) [omni-bencher] Make all runtimes work (#5872) Omni-Node renamings (#5915) ...

serban300 added the T0-node This PR/Issue is related to the topic “node”. label Oct 3, 2024

serban300 self-assigned this Oct 3, 2024

serban300 force-pushed the polkadot-parachain-renamings branch 5 times, most recently from a21aa14 to f977da2 Compare October 3, 2024 12:48

serban300 requested review from kianenigma and skunert October 3, 2024 13:15

kianenigma reviewed Oct 4, 2024

View reviewed changes

serban300 force-pushed the polkadot-parachain-renamings branch 3 times, most recently from aa30052 to b67524d Compare October 7, 2024 12:05

serban300 added 2 commits October 7, 2024 16:11

Omni-Node renamings

00c0dce

- moved the omni-node lib from `cumulus/polkadot-parachain/polkadot-parachain-lib` to `cumulus/polkadot-omni-node/lib` - renamed `polkadot-parachain-lib` to `polkadot-omni-node-lib`

Add polkadot-omni-node binary

e3bf5ad

serban300 force-pushed the polkadot-parachain-renamings branch from b67524d to 24e297f Compare October 7, 2024 13:15

Add prdoc

7ae8dd2

serban300 force-pushed the polkadot-parachain-renamings branch from 24e297f to 7ae8dd2 Compare October 7, 2024 13:26

skunert approved these changes Oct 8, 2024

View reviewed changes

Code review comment

f8c1875

kianenigma reviewed Oct 8, 2024

View reviewed changes

cumulus/polkadot-omni-node/Cargo.toml Outdated Show resolved Hide resolved

kianenigma reviewed Oct 8, 2024

View reviewed changes

cumulus/polkadot-omni-node/src/main.rs Show resolved Hide resolved

kianenigma approved these changes Oct 8, 2024

View reviewed changes

Docs improvements

589b5f0

Merge branch 'master' into polkadot-parachain-renamings

31dcc88

serban300 added this pull request to the merge queue Oct 8, 2024

Merged via the queue into paritytech:master with commit a4dce86 Oct 8, 2024
204 of 206 checks passed

serban300 deleted the polkadot-parachain-renamings branch October 8, 2024 12:49

serban300 mentioned this pull request Oct 8, 2024

polkadot-omni-node: re-namings #5566

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Omni-Node renamings #5915

Omni-Node renamings #5915

serban300 commented Oct 3, 2024 •

edited

Loading

skunert commented Oct 4, 2024

serban300 commented Oct 4, 2024

kianenigma commented Oct 4, 2024

kianenigma Oct 4, 2024

serban300 Oct 4, 2024

skunert commented Oct 4, 2024

kianenigma commented Oct 4, 2024 •

edited

Loading

serban300 commented Oct 7, 2024 •

edited

Loading

skunert left a comment

skunert Oct 8, 2024

serban300 Oct 8, 2024

Omni-Node renamings #5915

Omni-Node renamings #5915

Conversation

serban300 commented Oct 3, 2024 • edited Loading

skunert commented Oct 4, 2024

serban300 commented Oct 4, 2024

kianenigma commented Oct 4, 2024

kianenigma Oct 4, 2024

Choose a reason for hiding this comment

serban300 Oct 4, 2024

Choose a reason for hiding this comment

skunert commented Oct 4, 2024

kianenigma commented Oct 4, 2024 • edited Loading

serban300 commented Oct 7, 2024 • edited Loading

skunert left a comment

Choose a reason for hiding this comment

skunert Oct 8, 2024

Choose a reason for hiding this comment

serban300 Oct 8, 2024

Choose a reason for hiding this comment

serban300 commented Oct 3, 2024 •

edited

Loading

kianenigma commented Oct 4, 2024 •

edited

Loading

serban300 commented Oct 7, 2024 •

edited

Loading