LIU-412: Basic graph saving in DIM dlg/workspace #291
Conversation
…mmand line flag. - This is supported in cluster deployments, where the CLI flag is set for highest-level manager, and will therefore be stored on the node on which it is running.
- Previously, we had a work directory, but only stored the settings in there. This is because we never kept track of the user-provided work directory, and instead used the default. - We use a new DLG_WORKSPACE environment variable to maintain this state on the node
- This never actually worked as intended, as we didn't maintain the state of the working directory/workspace. (getDlgWorkDir always ended up being to DLG_ROOT\workspace) - This removes code previously added as part of this work to set the working directory, as we have decided there's no real need to maintain a workspace separate to the standard one.
Reviewer's Guide by SourceryThis PR implements graph storage functionality in DALiUGE by adding the ability to save physical graphs in the workspace directory. It also removes the deprecated working directory command line arguments, simplifying the workspace directory handling by always using DLG_ROOT/workspace. Sequence diagram for graph storage processsequenceDiagram
participant User
participant DIM
participant CompositeManager
participant FileSystem
User->>DIM: Submit graph
DIM->>CompositeManager: Add graphSpec
alt dump_graphs is true
CompositeManager->>FileSystem: _dump_graph_to_file(sessionId, graphSpec)
FileSystem-->>CompositeManager: Graph saved
end
CompositeManager-->>DIM: Graph processed
Class diagram for CompositeManager and related classesclassDiagram
class CompositeManager {
-list[str] dmHosts
-str pkeyPath
-int dmCheckTimeout
-bool dump_graphs
+_dump_graph_to_file(sessionId: str, graphSpec: dict)
}
class DataIslandManager {
-list[str] dmHosts
-str pkeyPath
-int dmCheckTimeout
-bool dump_graphs
}
class MasterManager {
-list[str] dmHosts
-str pkeyPath
-int dmCheckTimeout
-bool dump_graphs
}
CompositeManager <|-- DataIslandManager
CompositeManager <|-- MasterManager
note for CompositeManager "Added dump_graphs attribute and _dump_graph_to_file method"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @myxie - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -216,15 +203,14 @@ def start_dim(node_list, log_dir, origin_ip, logv=1): | |||
| args = [ | |||
There was a problem hiding this comment.
issue (code-quality): Replace interpolated string formatting with f-string (replace-interpolation-with-fstring)
| @@ -242,15 +228,14 @@ | |||
| args = [ | |||
There was a problem hiding this comment.
issue (code-quality): Replace interpolated string formatting with f-string (replace-interpolation-with-fstring)
There was a problem hiding this comment.
The changes look good, but I have two questions/concerns:
- It is not totally clear to me whether the previous default functionality to put the workspace under $DLG_ROOT is still maintained, since the get_workspace function, which returned that directory is now gone.
- Does this interfere with the start_dlg_cluster, which already puts everything into that directory?
|
Hi Andreas, We discussed this in person on Friday, but I thought I'd leave a detailed reply here for posterity and to clear up any lingering confusing that might exist.
The deleted daliuge/daliuge-common/dlg/utils.py Lines 212 to 218 in 9a3de7d As asked in your question, this will always sets the daliuge/daliuge-common/dlg/utils.py Lines 180 to 191 in 9a3de7d
This should not interfere with start_dlg_cluster, because the SlurmConfig that we create for Setonix sets the daliuge/daliuge-engine/dlg/deploy/configs/__init__.py Lines 161 to 163 in ff6f09b |
JIRA Ticket
LIU-412
Type
Problem/Issue
To progress towards re-running a graph, or to determine what the graph configuration was for a given session, we need to store graphs. Currently this is not something we can do in DALiUGE.
Solution
This PR adds an option for the DIM/MM to store the graphs in the DALiuGE workspace for the node that the DIM is running on. It is necessary to use the DIM/MM to do this, as NodeManagers only receive partitioned graphs and therefore do not have the entire "picture".
After discussion in-person, we have also made the decision to remove the
-w/--work-dircommand line argument, as it was not achieving the purpose for which it was intended (i.e. the working directory was intended to be the workspace, but we never set the workspace directory as anything other thanDLG_ROOT/workspace).Checklist
[ ] Unittests addedaddededitedSummary by Sourcery
Implement basic graph saving functionality in the DALiuGE workspace by adding an option to store graphs through the DIM/MM. Remove the '-w/--work-dir' command line argument to simplify configuration.
New Features:
Enhancements: