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

Add Async Backing guide #4363

Merged
merged 20 commits into from
Jun 28, 2024
Merged

Conversation

DrW3RK
Copy link
Contributor

@DrW3RK DrW3RK commented May 2, 2024

Update the instructions to work with the latest parachain template on Polkadot SDK

Update the instructions to work with the latest parachain template on Polkadot SDK
@DrW3RK
Copy link
Contributor Author

DrW3RK commented May 2, 2024

I have embedded the code from Parachain template wherever applicable using docify.

@kianenigma Perhaps the Async Guide upgrade doc is not a great candidate to showcase docify as most of the hardcoded example snippets serve as guidance for parachain devs to edit the code on their repositories which may be built from older versions of Substrate SDK. Here are a few pitfalls of using docify on this document:

  • Can't highlight specific lines like the example in step 1.4
  • Can't make edits to the exported code. This is problematic for guides like these, as it is very common that the readers will copy paste the snippet. Reference example - step 1.5 where the exported code shows "TRUE" and we want the reader to have it set to "FALSE"
  • I was unable to export "spawn-tasks" in step 2.2 and "AuraParams" in step 2.5
  • Can't show partial code snippet like in step 2.3. The function start_consensus is huge and we just want to instruct the reader to add a single line of extra code to it.
  • How to export the comments/documentation from the source code as well? docify just captures the actual code and ignores the documentation.

Any input on the above items is appreciated. Will be helpful for future docs.

@alexggh If you are able to do a walk through of the rendered doc and ensure its integrity, that would be of huge help. Thank you!

@kianenigma
Copy link
Contributor

Your observation is a bit unfortunate, but correct in that I might have been wrong in thinking that this doc is better demonstrated in docify.

Some of the points you mentioned can be improved by enhancing docify. For example:

  • #[docify::hidden] to hide unneeded stuff
  • #[docify::diff] to showcase just the diff of two snippets#4365

I had the intention to ask for a small treasury tip to incentivize @sam0x17 to implement some of these. But this is a midterm effort and I don't have a better solution in the short term.

My only suggestion in the short term is to not expect to re-use production code as-is in docify, and sometimes you might need to first relocate a subset of a working code in a new module/crate, and then use that.

Whether this is worth the effort or not, we will have to evaluate. Thank you for giving this a try in the meantime!

@DrW3RK
Copy link
Contributor Author

DrW3RK commented May 3, 2024

Sounds good! Docify is for sure very resourceful in embedding code, especially within the tutorial docs. If we are making a tutorial on Parachain template itself, I do see the potential of using docify.

I see you have created issues on the docify repo https://github.com/sam0x17/docify/issues Let's ask for a treasury tip to have them addressed!

Copy link
Contributor

@alexggh alexggh left a comment

Choose a reason for hiding this comment

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

The content is correct, but I think the rendering is a bit weird and makes it hard to pass through the guide.

docs/sdk/src/guides/parachains/async_backing_guide.rs Outdated Show resolved Hide resolved
@DrW3RK DrW3RK changed the title Update Async Backing docs using Docify Add Async Backing guide May 25, 2024
@DrW3RK
Copy link
Contributor Author

DrW3RK commented May 25, 2024

@kianenigma The doc is ready to be merged. Please review.

@kianenigma
Copy link
Contributor

I have some further ideas about how to improve, you can take a look here: DrW3RK/polkadot-sdk@async-backing-docs...paritytech:polkadot-sdk:kiz-feedback-kian-radha

@kianenigma
Copy link
Contributor

kianenigma commented Jun 13, 2024

@DrW3RK in the commit of mine that you have merged, I have demonstrated how you can use docify, but I have left the rest of it for you to do with the same mindset. Please follow the same pattern, and try and make as many of the code snippets as you can use real code.

@DrW3RK
Copy link
Contributor Author

DrW3RK commented Jun 13, 2024

on it

@DrW3RK
Copy link
Contributor Author

DrW3RK commented Jun 13, 2024

@kianenigma I've refined the document based on your feedback and used docify where it made sense.

On closer look, I observed that a few of the Docify imports in Phase 1 from your commit were misleading. As @alexggh mentioned here: #4363 (comment), this guide is for parachains that have not used the template and need to make modifications in multiple phases to activate the Async Backing feature.

For instance, with the usage of docify in Phase 1, the capacity is already 3 instead of 1, and the block time is 6000 instead of 12000.

image

I've attempted to make the code snippets concise throughout.

However, I failed to get this part of your commit to compile.

  1. Update [sp_consensus_aura::AuraApi::slot_duration] in [sp_api::impl_runtime_apis] to match the constant SLOT_DURATION

and got these errors. I can investigate this further, but if this is something trivial, please let me know. Thanks!

  = note: no item named `sp_consensus_aura` in scope
note: the lint level is defined here
   --> docs/sdk/src/guides/async_backing_guide.rs:252:9
    |
252 | #![deny(rustdoc::broken_intra_doc_links)]
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@alindima
Copy link
Contributor

As @alexggh mentioned here: #4363 (comment), this guide is for parachains that have not used the template and need to make modifications in multiple phases to activate the Async Backing feature.
For instance, with the usage of docify in Phase 1, the capacity is already 3 instead of 1, and the block time is 6000 instead of 12000.

This was also my point here: #4663 (comment)

I don't think we should be referencing the most up to date parachain template in the guide, because the template should already have all these features enabled.
These guides are IMO targeted at existing parachains that want to upgrade

@kianenigma
Copy link
Contributor

I see, this was a misunderstanding from my side though.

I personally think it is better to show someone the latest code and expect them to follow the instruction of "make it look like this", as opposed to trying to "imagine" what their code looks like, and then showing them exactly what line they have to change in the imaginary code.

But I will leave it up to you, and which one you and the rest of the parachains' team think it is a more worthwhile way to maintain it.

End of the day, all of my nitpicks here are in the direction of reducing the burdeon on all of you to not need to think about maintaining this. If you are okay with the task, our goal is reached and I'd be happy.

@DrW3RK
Copy link
Contributor Author

DrW3RK commented Jun 18, 2024

@kianenigma This was a good learning exercise for me on how Docify works. Happy to put this to use in Polkadot SDK docs.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6499208

minor fix - input args missing for impl_can_build_upon function
auto-merge was automatically disabled June 19, 2024 06:38

Head branch was pushed to by a user without write access

@DrW3RK
Copy link
Contributor Author

DrW3RK commented Jun 27, 2024

Could we add R0-silent and T11-documentation labels to this PR similar to #4244 ? I think that should take care of the failing CI jobs.

@alindima alindima added T11-documentation This PR/Issue is related to documentation. R0-silent Changes should not be mentioned in any release notes labels Jun 28, 2024
@alindima
Copy link
Contributor

Could we add R0-silent and T11-documentation labels to this PR similar to #4244 ? I think that should take care of the failing CI jobs.

Done. This can be queued for merge now

@kianenigma kianenigma added this pull request to the merge queue Jun 28, 2024
Merged via the queue into paritytech:master with commit 30cdf5d Jun 28, 2024
153 of 157 checks passed
TomaszWaszczyk pushed a commit to TomaszWaszczyk/polkadot-sdk that referenced this pull request Jul 7, 2024
Update the instructions to work with the latest parachain template on
Polkadot SDK

---------

Co-authored-by: kianenigma <kian@parity.io>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
TarekkMA pushed a commit to moonbeam-foundation/polkadot-sdk that referenced this pull request Aug 2, 2024
Update the instructions to work with the latest parachain template on
Polkadot SDK

---------

Co-authored-by: kianenigma <kian@parity.io>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T11-documentation This PR/Issue is related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants