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

(WIP) LIU-423: Initial work submitting subgraph through SlurmClient component. #299

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

myxie
Copy link
Collaborator

@myxie myxie commented Nov 21, 2024

JIRA Ticket

Type

  • Feature (addition)
  • Bug fix
  • Refactor (change)
  • Documentation

Problem/Issue

Solution

  • Successfully passed the subgraph to setonix
  • There are a few hacks that would be good to sort out in future discussions.

Checklist

  • Unittests added
    • Reason for not adding unittests (remove this line if added)
  • Documentation added
    • Reason for not adding documentation (remove this line if added)

Summary by Sourcery

Implement initial functionality for submitting a subgraph using the SlurmClient component, including handling subgraph data and updating the job submission process.

New Features:

  • Introduce the ability to submit a subgraph through the SlurmClient component.

Enhancements:

  • Add type annotations to the SlurmClient constructor parameters for better code clarity and type safety.

myxie and others added 8 commits November 12, 2024 22:46
(Incomplete)
- Basic use of SLURM 'template' script
- Skeleton use of .ini file for environment variables
- Added unittests to help prototyping
- Tests pass for config and slurm template
- Tested on Setonix
- Successfully passed the subgraph to setonix
- There are a few hacks that would be good to sort out in future discussions.
Copy link
Contributor

sourcery-ai bot commented Nov 21, 2024

Reviewer's Guide by Sourcery

This PR implements initial functionality for submitting subgraphs through the SlurmClient component. The changes focus on enhancing the SlurmClient class to handle subgraph submissions, improving type hints, and modifying the validation process in the translator REST API. The implementation includes temporary solutions that will need refinement in future updates.

Sequence diagram for subgraph submission through SlurmClient

sequenceDiagram
    participant Client
    participant SlurmClient
    participant FileSystem
    participant Remote

    Client->>SlurmClient: submit_job(subgraph)
    SlurmClient->>FileSystem: Write subgraph to /tmp/subgraph.graph
    SlurmClient->>SlurmClient: Set _logical_graph to /tmp/subgraph.graph
    SlurmClient->>SlurmClient: mk_session_dir()
    alt physical_graph_template_file exists
        SlurmClient->>FileSystem: Copy physical graph template
    else logical_graph exists
        SlurmClient->>Remote: Copy logical graph to remote
    end
    SlurmClient->>FileSystem: Create job submission script
    SlurmClient-->>Client: Return jobId
Loading

Sequence diagram for gen_pgt_post function

sequenceDiagram
    participant Client
    participant TranslatorREST

    Client->>TranslatorREST: gen_pgt_post(json_data)
    TranslatorREST->>TranslatorREST: Parse logical_graph
    alt Validation is commented out
        TranslatorREST->>TranslatorREST: Skip validation
    end
    TranslatorREST->>TranslatorREST: prepare_lgt(logical_graph, rmode)
    TranslatorREST-->>Client: Return prepared logical graph
Loading

Updated class diagram for SlurmClient

classDiagram
    class SlurmClient {
        -str dlg_root
        -str log_root
        -str host
        -str acc
        -str physical_graph_template_file
        -str logical_graph
        -int job_dur
        -int num_nodes
        -bool run_proxy
        -int mon_host
        -int mon_port
        -int logv
        -str facility
        -bool zerorun
        -int max_threads
        -bool sleepncopy
        -int num_islands
        -bool all_nics
        -bool check_with_session
        -bool submit
        -bool remote
        -str pip_name
        -str username
        +submit_job(subgraph: dict)
    }
Loading

File-Level Changes

Change Details Files
Enhanced SlurmClient class with subgraph submission capability
  • Added subgraph parameter to submit_job method
  • Implemented temporary subgraph storage in /tmp directory
  • Added logic to handle logical graph file copying for remote submissions
  • Made physical graph template file handling optional
daliuge-engine/dlg/deploy/slurm_client.py
Added type hints and default values to SlurmClient parameters
  • Added type annotations for all init parameters
  • Set default values for optional parameters
  • Updated parameter types to be more specific (str, int, bool)
daliuge-engine/dlg/deploy/slurm_client.py
Modified graph validation and processing
  • Temporarily disabled logical graph schema validation
  • Updated subgraph conversion logic to include subgraph field in application nodes
daliuge-translator/dlg/dropmake/web/translator_rest.py
daliuge-translator/dlg/dropmake/dm_utils.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@coveralls
Copy link

coveralls commented Nov 21, 2024

Coverage Status

coverage: 78.617% (-1.2%) from 79.795%
when pulling de16ba3 on LIU-423
into 20b29ec on master.

- Previous approach ended up using local graph as input to the script; this isn't how client.submit_job() works; it uses the remote path the phyiscal graph is expected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants