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

feat: model special resource requests too #512

Merged
merged 11 commits into from
Jul 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion martian-derive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
/// TODO:
/// - Handle default values for FileType
/// - Repo wide reorganization
extern crate proc_macro;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

was unneeded

use martian::{utils, MartianBlanketType, MartianPrimaryType, StageKind, Volatile, MARTIAN_TOKENS};
use quote::quote;
use std::collections::{HashMap, HashSet};
Expand Down Expand Up @@ -325,10 +324,10 @@

Err(Error::new(
span,
format!(
"{}. You are trying to use it on {} trait implementation.",
ATTR_NOT_ON_TRAIT_IMPL_ERROR, last_ident
),

Check warning on line 330 in martian-derive/src/lib.rs

View workflow job for this annotation

GitHub Actions / clippy_check

variables can be used directly in the `format!` string

warning: variables can be used directly in the `format!` string --> martian-derive/src/lib.rs:327:9 | 327 | / format!( 328 | | "{}. You are trying to use it on {} trait implementation.", 329 | | ATTR_NOT_ON_TRAIT_IMPL_ERROR, last_ident 330 | | ), | |_________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args = note: requested on the command line with `-W clippy::uninlined-format-args`
))
}

Expand Down Expand Up @@ -393,6 +392,7 @@
mem_gb: i16,
threads: i16,
vmem_gb: i16,
special: String,
volatile: Volatile,
stage_name: String
);
Expand Down Expand Up @@ -552,10 +552,10 @@
if blacklist.contains(name.as_str()) {
return syn::Error::new(
field.ident.unwrap().span(),
format!(
"Field name {} is not allowed here since it is a martian keyword",
name
),

Check warning on line 558 in martian-derive/src/lib.rs

View workflow job for this annotation

GitHub Actions / clippy_check

variables can be used directly in the `format!` string

warning: variables can be used directly in the `format!` string --> martian-derive/src/lib.rs:555:17 | 555 | / format!( 556 | | "Field name {} is not allowed here since it is a martian keyword", 557 | | name 558 | | ), | |_________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args
)
.to_compile_error()
.into();
Expand Down
2 changes: 1 addition & 1 deletion martian-derive/tests/ui_make_mro/attr_unknown_attr.stderr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: Expecting a comma separated `key=value` like tokens here. The allowed keys are: [mem_gb, threads, vmem_gb, volatile, stage_name]
error: Expecting a comma separated `key=value` like tokens here. The allowed keys are: [mem_gb, threads, vmem_gb, special, volatile, stage_name]
--> $DIR/attr_unknown_attr.rs:7:12
|
7 | #[make_mro(foo)]
Expand Down
6 changes: 2 additions & 4 deletions martian-lab/examples/sum_sq/src/sum_squares.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,16 @@
//! SumSquares stage code

use serde::{Deserialize, Serialize};

// The prelude brings the following items in scope:
// - Traits: MartianMain, MartianStage, RawMartianStage, MartianFileType, MartianMakePath
// - Struct/Enum: MartianRover, Resource, StageDef, MartianVoid,
// Error (from anyhow crate), LevelFilter (from log crate)
// - Macros: martian_stages!
// - Functions: martian_main, martian_main_with_log_level, martian_make_mro
use martian::prelude::*;

// Bring the procedural macros in scope:
// #[derive(MartianStruct)], #[derive(MartianType)], #[make_mro], martian_filetype!
use martian_derive::{make_mro, MartianStruct};
use serde::{Deserialize, Serialize};

// NOTE: The following four structs will serve as the associated type for the
// trait. The struct fields need to be owned and are limited to
Expand Down Expand Up @@ -73,7 +71,7 @@ impl MartianStage for SumSquares {
for value in args.values {
let chunk_inputs = SumSquaresChunkInputs { value };
// It is optional to create a chunk with resource. If not specified, default resource will be used
stage_def.add_chunk_with_resource(chunk_inputs, chunk_resource);
stage_def.add_chunk_with_resource(chunk_inputs, chunk_resource.clone());
}
// Return the stage definition
Ok(stage_def)
Expand Down
34 changes: 18 additions & 16 deletions martian-lab/tests/error_test/error_test.json
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
{
"command": [
"run_test.sh"
],
"expected_return": 1,
"output_dir": "pipeline_test",
"contains_only_files": [
"*"
],
"contents_match": [],
"file_contains_string": [
{
"file": "SUM_SQUARES/SUM_SQUARES/fork0/chnk3/_stackvars",
"string": "martian-rust/martian-lab/examples/sum_sq/src/sum_squares.rs:94"
}
]
}
"command": [
"run_test.sh"
],
"expected_return": 1,
"output_dir": "pipeline_test",
"contains_only_files": [
"*"
],
"contents_match": [

],
"file_contains_string": [
{
"file": "SUM_SQUARES/SUM_SQUARES/fork0/chnk3/_stackvars",
"string": "martian-rust/martian-lab/examples/sum_sq/src/sum_squares.rs:92"
}
]
}
Original file line number Diff line number Diff line change
@@ -1,44 +1,45 @@
0: anyhow::private::format_err
at ./cargo/registry/src/github.com-1ecc6299db9ec823/anyhow-1.0.51/src/lib.rs:659:13
1: <sum_sq::sum_squares::SumSquares as martian::stage::MartianStage>::main
at ./martian-lab/examples/sum_sq/src/sum_squares.rs:94:24
2: <T as martian::stage::RawMartianStage>::main
at ./martian/src/stage.rs:681:20
3: martian::martian_entry_point
at ./martian/src/lib.rs:262:9
4: martian::MartianAdapter<S>::run_get_error
at ./martian/src/lib.rs:164:9
5: martian::MartianAdapter<S>::run
at ./martian/src/lib.rs:157:9
6: sum_sq::main
at ./martian-lab/examples/sum_sq/src/main.rs:48:23
7: core::ops::function::FnOnce::call_once
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/ops/function.rs:227:5
8: std::sys_common::backtrace::__rust_begin_short_backtrace
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/std/src/sys_common/backtrace.rs:125:18
9: std::rt::lang_start::{{closure}}
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/std/src/rt.rs:63:18
10: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/core/src/ops/function.rs:259:13
std::panicking::try::do_call
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/std/src/panicking.rs:401:40
std::panicking::try
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/std/src/panicking.rs:365:19
std::panic::catch_unwind
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/std/src/panic.rs:434:14
std::rt::lang_start_internal::{{closure}}
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/std/src/rt.rs:45:48
std::panicking::try::do_call
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/std/src/panicking.rs:401:40
std::panicking::try
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/std/src/panicking.rs:365:19
std::panic::catch_unwind
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/std/src/panic.rs:434:14
std::rt::lang_start_internal
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/std/src/rt.rs:45:20
11: std::rt::lang_start
at /rustc/c8dfcfe046a7680554bf4eb612bad840e7631c4b/library/std/src/rt.rs:62:5
12: main
13: __libc_start_main
14: <unknown>
at /sysdeps/x86_64/elf/start.S:103
0: anyhow::error::<impl anyhow::Error>::msg
at /mnt/bazelbuild/user/preyas.shah/cargo/registry/src/index.crates.io-6f17d22bba15001f/anyhow-1.0.86/src/backtrace.rs:27:14
1: anyhow::__private::format_err
at /mnt/bazelbuild/user/preyas.shah/cargo/registry/src/index.crates.io-6f17d22bba15001f/anyhow-1.0.86/src/lib.rs:689:13
2: <sum_sq::sum_squares::SumSquares as martian::stage::MartianStage>::main
at /mnt/home/preyas/Reps/martian-rust/martian-lab/examples/sum_sq/src/sum_squares.rs:92:24
3: <T as martian::stage::RawMartianStage>::main
at /mnt/home/preyas/Reps/martian-rust/martian/src/stage.rs:778:20
4: martian::martian_entry_point
at /mnt/home/preyas/Reps/martian-rust/martian/src/lib.rs:261:9
5: martian::MartianAdapter<S>::run_get_error
at /mnt/home/preyas/Reps/martian-rust/martian/src/lib.rs:165:9
6: martian::MartianAdapter<S>::run
at /mnt/home/preyas/Reps/martian-rust/martian/src/lib.rs:158:9
7: sum_sq::main
at /mnt/home/preyas/Reps/martian-rust/martian-lab/examples/sum_sq/src/main.rs:48:23
8: core::ops::function::FnOnce::call_once
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/ops/function.rs:250:5
9: std::sys_common::backtrace::__rust_begin_short_backtrace
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/sys_common/backtrace.rs:155:18
10: std::rt::lang_start::{{closure}}
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/rt.rs:159:18
11: core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/ops/function.rs:284:13
12: std::panicking::try::do_call
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/panicking.rs:559:40
13: std::panicking::try
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/panicking.rs:523:19
14: std::panic::catch_unwind
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/panic.rs:149:14
15: std::rt::lang_start_internal::{{closure}}
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/rt.rs:141:48
16: std::panicking::try::do_call
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/panicking.rs:559:40
17: std::panicking::try
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/panicking.rs:523:19
18: std::panic::catch_unwind
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/panic.rs:149:14
19: std::rt::lang_start_internal
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/rt.rs:141:20
20: std::rt::lang_start
at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/rt.rs:158:17
21: main
22: __libc_start_main
23: <unknown>
Original file line number Diff line number Diff line change
Expand Up @@ -2,25 +2,29 @@
"chunks": [
{
"__mem_gb": 1,
"__special": null,
Copy link
Member

Choose a reason for hiding this comment

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

This should not be written if special was unset. Please fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"__threads": 1,
"__vmem_gb": null,
"value": 1.0
},
{
"__mem_gb": 1,
"__special": null,
"__threads": 1,
"__vmem_gb": null,
"value": 2.0
},
{
"__mem_gb": 1,
"__special": null,
"__threads": 1,
"__vmem_gb": null,
"value": 3.0
}
],
"join": {
"__mem_gb": null,
"__special": null,
"__threads": null,
"__vmem_gb": null
}
Expand Down
6 changes: 3 additions & 3 deletions martian/src/mro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,12 @@
match field_width {
Some(width) => {
let min_width = self.min_width();
assert!(
width >= min_width,
"Need a minimum width of {}. Found {}",
min_width,
width
);

Check warning on line 41 in martian/src/mro.rs

View workflow job for this annotation

GitHub Actions / clippy_check

variables can be used directly in the `format!` string

warning: variables can be used directly in the `format!` string --> martian/src/mro.rs:36:17 | 36 | / assert!( 37 | | width >= min_width, 38 | | "Need a minimum width of {}. Found {}", 39 | | min_width, 40 | | width 41 | | ); | |_________________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args = note: requested on the command line with `-W clippy::uninlined-format-args`
self.mro_string_with_width(width)
}
None => self.mro_string_no_width(),
Expand Down Expand Up @@ -110,7 +110,7 @@

for field in &self.fields {
let formatted_line = field.fmt_align_4_columns(ty_width, name_width, desc_width);
writeln!(f, " {},", formatted_line)?;

Check warning on line 113 in martian/src/mro.rs

View workflow job for this annotation

GitHub Actions / clippy_check

variables can be used directly in the `format!` string

warning: variables can be used directly in the `format!` string --> martian/src/mro.rs:113:13 | 113 | writeln!(f, " {},", formatted_line)?; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args help: change this to | 113 - writeln!(f, " {},", formatted_line)?; 113 + writeln!(f, " {formatted_line},")?; |
}
writeln!(f, ")")
}
Expand Down Expand Up @@ -523,11 +523,11 @@
// Check that name does not match any martian token.
fn verify(&self) {
for &token in MARTIAN_TOKENS {
assert!(
self.name != token,
"Martian token {} cannot be used as field name",
token
);

Check warning on line 530 in martian/src/mro.rs

View workflow job for this annotation

GitHub Actions / clippy_check

variables can be used directly in the `format!` string

warning: variables can be used directly in the `format!` string --> martian/src/mro.rs:526:13 | 526 | / assert!( 527 | | self.name != token, 528 | | "Martian token {} cannot be used as field name", 529 | | token 530 | | ); | |_____________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args
}
assert!(!self.name.starts_with("__"));
}
Expand Down Expand Up @@ -602,7 +602,7 @@
/// threads = 16,
/// )
/// ```
#[derive(Debug, Default, Clone, Copy, Serialize, Deserialize)]
#[derive(Debug, Default, Clone, Serialize, Deserialize)]
pub struct MroUsing {
$(pub $property: Option<$type>,)*
}
Expand All @@ -622,7 +622,7 @@
return Ok(());
}
$(
if let Some($property) = self.$property {
if let Some($property) = self.$property.clone() {
writeln!(
f,
"{blank:indent$}{key:<width$} = {value},",
Expand Down Expand Up @@ -654,7 +654,7 @@
};
}

mro_using! {mem_gb: i16, threads: i16, vmem_gb: i16, volatile: Volatile}
mro_using! {mem_gb: i16, threads: i16, vmem_gb: i16, special: String, volatile: Volatile}

/// Input and outputs fields together
#[derive(Debug, Default)]
Expand Down Expand Up @@ -923,12 +923,12 @@

if let Some(ref chunk_in_out) = self.minified_chunk_in_outs() {
writeln!(f, ") split (")?;
write!(
f,
"{params:<ty_width$}",
params = chunk_in_out,
ty_width = ty_width
)?;

Check warning on line 931 in martian/src/mro.rs

View workflow job for this annotation

GitHub Actions / clippy_check

variables can be used directly in the `format!` string

warning: variables can be used directly in the `format!` string --> martian/src/mro.rs:926:13 | 926 | / write!( 927 | | f, 928 | | "{params:<ty_width$}", 929 | | params = chunk_in_out, 930 | | ty_width = ty_width 931 | | )?; | |_____________^ | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#uninlined_format_args
}

if self.using_attrs.need_using() {
Expand Down
28 changes: 26 additions & 2 deletions martian/src/stage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,16 @@ impl<T: MartianFileType> MartianMakePath for T {
///
/// Memory/ thread request can be negative in matrian. See
/// [http://martian-lang.org/advanced-features/#resource-consumption](http://martian-lang.org/advanced-features/#resource-consumption)
#[derive(Debug, Serialize, Deserialize, Copy, Clone, Default)]
#[derive(Debug, Serialize, Deserialize, Clone, Default)]
pub struct Resource {
#[serde(rename = "__mem_gb")]
mem_gb: Option<isize>,
#[serde(rename = "__threads")]
threads: Option<isize>,
#[serde(rename = "__vmem_gb")]
vmem_gb: Option<isize>,
#[serde(rename = "__special")]
special: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

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

This cannot be specified in the using section of the mro right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It can be specified

Copy link
Member

Choose a reason for hiding this comment

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

Okay, to support it in the using section, we would need to update

mro_using! {mem_gb: i16, threads: i16, vmem_gb: i16, volatile: Volatile}
and . This is only needed if we want to specify #[make_mro(special="...")]

Copy link
Member

Choose a reason for hiding this comment

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

This should really be

Suggested change
special: Option<String>,
special: Option<&' static str>,

There is no plausible use case for this being something other than a compile-time constant.

Copy link
Member

Choose a reason for hiding this comment

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

(yes, I know this poses problems for deserialization but I can't think of a plausible case where you need to deserialize it either)

}

impl Resource {
Expand Down Expand Up @@ -164,6 +166,11 @@ impl Resource {
self.threads
}

/// Get the special resource request
pub fn get_special(&self) -> Option<String> {
self.special.clone()
preyasshah marked this conversation as resolved.
Show resolved Hide resolved
}

/// Set the mem_gb
/// ```rust
/// use martian::Resource;
Expand Down Expand Up @@ -206,6 +213,21 @@ impl Resource {
self
}

/// Set the special request
/// ```rust
/// use martian::Resource;
///
/// let resource = Resource::new().mem_gb(2).vmem_gb(4).special("gpu_count1_mem8".to_owned());
/// assert_eq!(resource.get_mem_gb(), Some(2));
/// assert_eq!(resource.get_vmem_gb(), Some(4));
/// assert_eq!(resource.get_threads(), None);
/// assert_eq!(resource.get_special(), Some("gpu_count1_mem8".to_owned()));
/// ```
pub fn special(mut self, special: String) -> Self {
self.special = Some(special);
self
}

/// Create a resource with the specified `mem_gb`. `vmem_gb` and
/// `threads` are set to None.
///
Expand All @@ -222,6 +244,7 @@ impl Resource {
mem_gb: Some(mem_gb),
threads: None,
vmem_gb: None,
special: None,
}
}

Expand All @@ -241,6 +264,7 @@ impl Resource {
mem_gb: None,
threads: Some(threads),
vmem_gb: None,
special: None,
}
}
}
Expand Down Expand Up @@ -609,7 +633,7 @@ pub trait MartianStage: MroMaker {
fill_defaults(resource),
))
}
let rover = _chunk_prelude(chunk_idx, run_directory, chunk.resource)?;
let rover = _chunk_prelude(chunk_idx, run_directory, chunk.resource.clone())?;
self.main(args.clone(), chunk.inputs.clone(), rover)
};

Expand Down
Loading