Skip to content
Open
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 Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "pharos"
version = "0.3.0"
version = "0.3.1"
edition = "2024"

[[bin]]
Expand Down
54 changes: 53 additions & 1 deletion components/config/src/nonmem.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::cmp::Ordering;
use std::collections::HashMap;
use std::path::{Path, PathBuf};
use std::process::Command;
Expand All @@ -10,6 +11,28 @@ use which::which;

const KNOWN_NONMEM_FOLDERS: [&str; 2] = ["/opt/nonmem", "/opt/NONMEM"];

fn parse_nonmem_version_name(name: &str) -> Option<u32> {
let rest = name.strip_prefix("nm")?;
let digit_count = rest.chars().take_while(|c| c.is_ascii_digit()).count();
if digit_count == 0 {
return None;
}

rest[..digit_count].parse::<u32>().ok()
}

fn cmp_nonmem_version_names(a: &str, b: &str) -> Ordering {
let a_num = parse_nonmem_version_name(a);
let b_num = parse_nonmem_version_name(b);

match (a_num, b_num) {
(Some(a_num), Some(b_num)) => b_num.cmp(&a_num).then_with(|| a.cmp(b)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need the then_with?

Copy link
Collaborator Author

@mduncans mduncans Mar 6, 2026

Choose a reason for hiding this comment

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

without it, it could put versions with the compiler information before the plain nm7x.

assertion `left == right` failed
  left: ["nm76", "nm75", "nm74gf_nmfe", "nm74", "nm74gf", "nm73", "nm73gf", "nm73gf_nmfe"]
 right: ["nm76", "nm75", "nm74", "nm74gf", "nm74gf_nmfe", "nm73", "nm73gf", "nm73gf_nmfe"]

I'm not sure if this is a concern in practice, but feels like it nm7x should take precedence over nm7x_y

(Some(_), None) => Ordering::Less,
(None, Some(_)) => Ordering::Greater,
(None, None) => a.cmp(b),
}
}

fn resolve_path_from_config_dir(path: Option<&PathBuf>, config_dir: &Path) -> Option<PathBuf> {
path.map(|p| {
if p.is_relative() {
Expand Down Expand Up @@ -432,7 +455,7 @@ impl NonmemConfig {
}
config.versions = find_nonmem_versions()?;
let mut versions = config.versions.keys().collect::<Vec<_>>();
versions.sort();
versions.sort_by(|a, b| cmp_nonmem_version_names(a, b));
config.default_version = versions[0].clone();

Ok(config)
Expand Down Expand Up @@ -646,4 +669,33 @@ mod tests {
let nonmem = config.nonmem.expect("Should have nonmem config");
assert_eq!(nonmem.files_to_copy().len(), 0);
}

#[test]
fn test_nonmem_versions_sorted_latest_first_with_lexical_fallback() {
let mut versions = [
"nm74gf_nmfe",
"nm75",
"nm74",
"nm73",
"nm73gf",
"nm74gf",
"nm73gf_nmfe",
"nm76",
];
versions.sort_by(|a, b| cmp_nonmem_version_names(a, b));

assert_eq!(
versions,
[
"nm76",
"nm75",
"nm74",
"nm74gf",
"nm74gf_nmfe",
"nm73",
"nm73gf",
"nm73gf_nmfe"
]
);
}
}
53 changes: 45 additions & 8 deletions components/scheduler/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,11 +270,24 @@ impl SchedulerType {
.with_context(
|| format!("failed to execute {cmd_name} command for model {m:?}",),
)?;

// If SGE does not have a compute node ready during job submission
// qsub fails and gives this error:
//
// Unable to run job: warning: <your-user-name's> job is not allowed to run in any queue
// Your job <number> ("<model-name>") has been submitted
// Exiting.
//
// Checking stderr for this message to prevent bail! on non-zero exit code from qsub
if !output.status.success() {
bail!(
"{cmd_name} failed: {}",
String::from_utf8_lossy(&output.stderr)
);
let stderr = String::from_utf8_lossy(&output.stderr);
let sge_queuing_warning = stderr.contains("Unable to run job: warning")
&& stderr.contains("job is not allowed to run in any queue")
&& stderr.contains("has been submitted");
if !sge_queuing_warning {
bail!("{cmd_name} failed: {stderr}");
}
log::warn!("{cmd_name} reported a warning but the job was submitted: {stderr}");
}

let stdout = String::from_utf8_lossy(&output.stdout);
Expand All @@ -284,10 +297,34 @@ impl SchedulerType {
num.parse()
.map_err(|e| anyhow!("Failed to parse job ID '{stdout}': {e}"))?
}
SchedulerType::Sge(_) => stdout
.trim()
.parse()
.map_err(|e| anyhow!("Failed to parse job ID '{stdout}': {e}"))?,
SchedulerType::Sge(_) => {
// If qsub failed due to no compute nodes being available,
// the job ID is not printed in stdout but rather in the
// error message given to stderr (see error message template above).
if !stdout.trim().is_empty() {
stdout
.trim()
.parse()
.map_err(|e| anyhow!("Failed to parse job ID '{stdout}': {e}"))?
} else {
// Need to isolate job ID from
// Your job <number> ("<model-name>") has been submitted
let stderr = String::from_utf8_lossy(&output.stderr);
let job_id_str = stderr
.lines()
.find_map(|line| {
line.trim()
.strip_prefix("Your job ")
.and_then(|rest| rest.split_whitespace().next())
})
.ok_or_else(|| {
anyhow!("Failed to find job ID in SGE output: {stderr}")
})?;
job_id_str
.parse()
.map_err(|e| anyhow!("Failed to parse job ID '{job_id_str}': {e}"))?
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this block needs some comment

}
};

out.push((m, job_id));
Expand Down
2 changes: 1 addition & 1 deletion components/scheduler/src/sge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use tera::Tera;
const DEFAULT_TEMPLATE: &str = r#"#!/bin/bash
#$ -N {{job_name}}
#$ -V
#$ -j oe
#$ -j y
#$ -o {{log_path}}
{% if parallel -%}#$ -pe orte {{num_mpi_cpus}}{% endif %}

Expand Down