Add support for Pbs #69
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds comprehensive PBS scheduler support across the codebase, including model adjustments, exception handling, client integration, API schema extensions, updated shell scripts for job submission, and Docker-based PBS cluster provisioning.
- Core models and exceptions updated to support PBS entities and optional fields.
- Firecrest API endpoints and response models extended to include PBS types.
- Shell scripts now fall back to PBS_JOBID, and Docker Compose plus Dockerfiles configured for a local PBS cluster.
Reviewed Changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/scheduler_clients/models.py | Made JobTime.limit optional to accommodate missing values. |
| src/lib/scheduler_clients/init.py | Imported PBS models and updated __all__. |
| src/lib/exceptions.py | Introduced new PbsError exception subclass. |
| src/firecrest/status/models.py | Extended node/partition/reservation responses to include PBS types. |
| src/firecrest/filesystem/transfer/scripts/slurm_job_move.sh | Added PBS_JOBID fallback in echo statement. |
| src/firecrest/filesystem/transfer/scripts/slurm_job_extract.sh | Added PBS_JOBID fallback in echo statement. |
| src/firecrest/filesystem/transfer/scripts/slurm_job_downloader.sh | Added PBS_JOBID fallback in echo statement. |
| src/firecrest/filesystem/transfer/scripts/slurm_job_delete.sh | Added PBS_JOBID fallback in echo statement. |
| src/firecrest/filesystem/transfer/scripts/slurm_job_copy.sh | Added PBS_JOBID fallback in echo statement. |
| src/firecrest/filesystem/transfer/scripts/slurm_job_compress.sh | Added PBS_JOBID fallback in echo statement. |
| src/firecrest/filesystem/transfer/router.py | Added trailing commas and formatting; duplicated work_dir logic. |
| src/firecrest/dependencies.py | Registered PbsCliClient for SchedulerType.pbs. |
| src/firecrest/config.py | Added pbs to SchedulerType enum. |
| src/firecrest/compute/models.py | Extended submit/response models to include PBS job types. |
| f7t-api-config.local-env.yaml | Added a cluster-pbs entry with PBS-specific directives. |
| docker-compose.yml | Added pbs service and commented-out platform entry. |
| build/docker/pbs-cluster/supervisord.conf | Created supervisord configuration for PBS services. |
| build/docker/pbs-cluster/ssh/sshd_config_base | Base SSHD config with PBS cluster settings. |
| build/docker/pbs-cluster/init_sequence.sh | Initialization script for PBS postinstall and service startup. |
| build/docker/pbs-cluster/Dockerfile | Dockerfile to build an Ubuntu-based PBS cluster image. |
src/firecrest/filesystem/transfer/scripts/slurm_job_downloader.sh
Outdated
Show resolved
Hide resolved
src/firecrest/filesystem/transfer/scripts/slurm_job_compress.sh
Outdated
Show resolved
Hide resolved
| work_dir = next( | ||
| iter([fs.path for fs in system.file_systems if fs.default_work_dir]), None | ||
| ) |
There was a problem hiding this comment.
[nitpick] The logic to compute work_dir using next(iter(...), None) is repeated multiple times; consider extracting it into a helper function to reduce duplication.
| work_dir = next( | |
| iter([fs.path for fs in system.file_systems if fs.default_work_dir]), None | |
| ) | |
| work_dir = _get_default_work_dir(system.file_systems) |
Copilot uses AI. Check for mistakes.
|
|
||
| RUN useradd -ms /bin/bash pbsuser | ||
|
|
||
| RUN apt-get update && apt-get install -y postgresql postgresql-contrib && rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
There are multiple consecutive apt-get update commands; consider combining package installs into a single RUN block to reduce image layers and speed up builds.
Copilot uses AI. Check for mistakes.
|
|
||
|
|
||
| PubkeyAcceptedKeyTypes ssh-rsa,ssh-rsa-cert-v01@openssh.com,ssh-ed25519-cert-v01@openssh.com,ssh-ed25519 | ||
| PermitRootLogin yes |
There was a problem hiding this comment.
Duplicate PermitRootLogin directive found (also on line 23); consolidate or remove redundant entries to avoid ambiguity.
| PermitRootLogin yes |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This is still work-in-progress