feat: martian-pipestance crate #412
Conversation
sreenathkrishnan
requested review from
macklin-10x,
adam-azarchs and
sjackman
| @@ -0,0 +1,308 @@ | |||
| //! | |||
| //! Critical path in the pipestance | |||
There was a problem hiding this comment.
It would be nice to flesh out this docstring a little bit to make it clear exactly what we're talking about here.
| weight_function, | ||
| )) | ||
| } | ||
| pub fn compute(final_state: &FinalState, perf: &Perf, weight_function: StageWeight) -> Self { |
There was a problem hiding this comment.
Bikeshedding but this is kind of just a constructor so new is probably the conventional name.
|
|
||
| fn new() -> Self { | ||
| Self::default() | ||
| } |
There was a problem hiding this comment.
Providing new when we already derive Default isn't necessary IMHO.
| fn new() -> Self { | |
| Self::default() | |
| } |
| } | ||
| } | ||
|
|
||
| fn _critical_path(mut self) -> CriticalPath { |
There was a problem hiding this comment.
Leading underscore method names aren't idiomatic in Rust; even then, this method is called and is already marked private.
| fn _critical_path(mut self) -> CriticalPath { | |
| fn critical_path(mut self) -> CriticalPath { |
| let critical_path = | ||
| CriticalPath::compute(&final_state, &perf, StageWeight::NoQueueWallTime); | ||
|
|
||
| println!("{:#?}", critical_path); |
There was a problem hiding this comment.
Can you please address the few clippy warnings?
| use serde::de::DeserializeOwned; | ||
| use std::path::Path; | ||
|
|
||
| pub mod common; |
There was a problem hiding this comment.
I think it would make more sense to move the things in common into the crate root and mark them pub(crate).
| @@ -0,0 +1,26 @@ | |||
| use anyhow::{Context, Result}; | |||
There was a problem hiding this comment.
I think this crate should expose an explicit API by pub useing the relevant types/functions.
| pub mod critical_path; | ||
| pub mod final_state; | ||
| pub mod perf; |
There was a problem hiding this comment.
Should these actually be pub or should we re-export the relevant pieces?
| /// A file associated with a martian pipestance such as _perf, _finalstate etc | ||
| /// that can be deserialized into a concrete type | ||
| pub trait PipestanceFile: DeserializeOwned { | ||
| fn filename() -> &'static str; |
There was a problem hiding this comment.
Since this is 'static you might as well make this a const member instead of a method, unless we think this will need to be dynamically dispatched in certain cases.
| fn from_string(file_contents: String) -> Result<Self> { | ||
| Ok(serde_json::from_str(&file_contents)?) | ||
| } |
There was a problem hiding this comment.
Since this is actually parsing JSON, it should probably be named accordingly. Also, it doesn't need to take ownership of the input.
| fn from_string(file_contents: String) -> Result<Self> { | |
| Ok(serde_json::from_str(&file_contents)?) | |
| } | |
| fn from_json(file_contents: &str) -> Result<Self> { | |
| Ok(serde_json::from_str(file_contents)?) | |
| } |
There was a problem hiding this comment.
I think it's a really bad idea to reinvent this wheel. There's a lot of subtitles that need to be gotten right. We already have code to do all of this.
It also probably shouldn't be part of the martian-lang/martian-rust repo, which is all about support for writing stage code in rust.
| @@ -3,3 +3,4 @@ martian-filetypes/example.bincode | |||
| martian-filetypes/example.json | |||
| martian-filetypes/example_lazy.bincode | |||
| martian-filetypes/example_lazy.json | |||
| .DS_Store | |||
There was a problem hiding this comment.
Newline at end of file please.
| # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html | ||
|
|
||
| [dependencies] | ||
| serde = { version = "1.0", features = ['derive'] } |
There was a problem hiding this comment.
Please be consistent about which quotes to use.
|
I'm going to close this PR. It doesn't belong in this repo, it doesn't product correct output, and it's redundant with existing code we have elsewhere. |
|
@adam-azarchs can you please link this PR to the aforementioned existing code for our current edification/posterity? |
I am okay not including the critical path code here, but it makes sense to have the struct definitions for perf and finalstate in this repo. |
Why? Nothing using this repo should be touching top-level pipestance state like that. |
|
So I see library code there, but is there an existing executable to output the critical path from _perf data? If not, at least some guidance to how to fit the existing pieces together would probably be useful. |
|
Yes, but they needed a bit of maintenance; see the (private repo) PRs I sent you. |
_perfand_finalstatefile. Code mostly copied from Logan's crate