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

Pvf refactor execute worker errors follow up #4071

Conversation

maksimryndin
Copy link
Contributor

@maksimryndin maksimryndin commented Apr 10, 2024

follow up of #2604
closes #2604

  • take relevant changes from Marcin's PR
  • extract common duplicate code for workers (low-hanging fruits)

Some unpassed ci problems are more general and should be fixed in master (see #4074)

Proposed labels: T0-node, R0-silent, I4-refactor


kusama address: FZXVQLqLbFV2otNXs6BMnNch54CFJ1idpWwjMb3Z8fTLQC6

@maksimryndin maksimryndin marked this pull request as ready for review April 11, 2024 12:30
@s0me0ne-unkn0wn s0me0ne-unkn0wn added R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”. labels Apr 11, 2024
Copy link
Contributor

@s0me0ne-unkn0wn s0me0ne-unkn0wn left a comment

Choose a reason for hiding this comment

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

This looks great, thanks a lot!

@@ -136,6 +136,8 @@ pub enum InternalValidationError {
/// Could not find or open compiled artifact file.
#[error("validation: could not find or open compiled artifact file: {0}")]
CouldNotOpenFile(String),
#[error("validation: could not create pipe: {0}")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Docstring is missing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!) Fixed 5bdda49

@s0me0ne-unkn0wn
Copy link
Contributor

/tip small

Copy link

@s0me0ne-unkn0wn You are not allowed to request a tip. Only members of paritytech/tip-bot-approvers are allowed.

@s0me0ne-unkn0wn
Copy link
Contributor

Oh you don't say

Copy link
Contributor

@AndreiEres AndreiEres left a comment

Choose a reason for hiding this comment

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

Good!

pin-project = "1.0.9"
rand = "0.8.5"
slotmap = "1.0"
tempfile = "3.3.0"
thiserror = { workspace = true }
tokio = { version = "1.24.2", features = ["fs", "process"] }

parity-scale-codec = { version = "3.6.1", default-features = false, features = ["derive"] }
parity-scale-codec = { version = "3.6.1", default-features = false, features = [
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, different formatter settings here and below

Copy link
Contributor Author

@maksimryndin maksimryndin Apr 17, 2024

Choose a reason for hiding this comment

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

Not sure if I've got it right, but it is the result of applying taplo fmt polkadot/node/core/pvf/Cargo.toml --config .config/taplo.toml as in the ci job

@@ -127,13 +137,23 @@ pub fn worker_entrypoint(
|mut stream, worker_info, security_status| {
let artifact_path = worker_dir::execute_artifact(&worker_info.worker_dir_path);

let Handshake { executor_params } = recv_execute_handshake(&mut stream)?;
let Handshake { executor_params } = map_err!(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: what do you think if we rewrite it something like that?

let Handshake { executor_params } = recv_execute_handshake(&mut stream)
    .map_err(|e| map_and_send_err!(e, InternalValidationError::HostCommunication, &mut stream, worker_info))?;

More words, but easier to read imho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure) thanks:) refactored at a05897f

@bkchr
Copy link
Member

bkchr commented Apr 17, 2024

/tip small

Copy link

@bkchr A referendum for a small (4 KSM) tip was successfully submitted for @maksimryndin (FZXVQLqLbFV2otNXs6BMnNch54CFJ1idpWwjMb3Z8fTLQC6 on kusama).

Referendum number: 378.
tip

Copy link

The referendum has appeared on Polkassembly.

@s0me0ne-unkn0wn s0me0ne-unkn0wn added this pull request to the merge queue Apr 19, 2024
Merged via the queue into paritytech:master with commit 4eabe5e Apr 19, 2024
134 of 137 checks passed
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 T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants