Docker fix #294
Docker fix #294
Conversation
Reviewer's Guide by SourceryThis PR fixes a bug in the DockerApp component where arguments were incorrectly passed to containers with pre-defined entrypoints. The fix modifies how entrypoints and commands are handled, making it possible to use default entrypoints with additional arguments. The PR also includes several deployment-related improvements and refactoring of configuration handling. Updated class diagram for MPIApp and DockerAppclassDiagram
class MPIApp {
-command: String
-maxprocs: Integer
-use_wrapper: Boolean
-args: String
-applicationArgs: Map
-argumentPrefix: String
-paramValueSeparator: String
-inputRedirect: String
-outputRedirect: String
-recompute_data: Map
+initialize(**kwargs)
+run()
}
class DockerApp {
-entryPoint: String
-defaultEntryPoint: String
+initialize(**kwargs)
+run()
}
MPIApp --> BarrierAppDROP
DockerApp --> BarrierAppDROP
note for MPIApp "Added new attributes for argument handling and input/output redirection"
note for DockerApp "Modified entryPoint handling to allow default entrypoints"
Updated class diagram for ConfigFactory and SlurmClientclassDiagram
class ConfigFactory {
+create_config(facility: String, user: String)
}
class SlurmClient {
-config: DefaultConfig
-host: String
-acc: String
-user: String
-dlg_root: String
-log_root: String
-modules: String
-venv: String
-exec_prefix: String
+create_job_desc(physical_graph_file: String)
+mk_session_dir(dlg_root: String)
}
ConfigFactory --> DefaultConfig
SlurmClient --> ConfigFactory
note for ConfigFactory "Added user parameter to create_config method"
note for SlurmClient "Added exec_prefix attribute and updated initialization"
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 @awicenec - I've reviewed your changes - here's some feedback:
Overall Comments:
- While the Docker-related changes look good, consider splitting unrelated deployment config changes into a separate PR for clearer review and history tracking.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 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.
| print( | ||
| f"ERROR: Unable to create {session_dir} on {self.username}@{self.host}" | ||
| ) | ||
| sys.exit() |
There was a problem hiding this comment.
issue: Improve error handling for SSH exceptions
Instead of catching exceptions and calling sys.exit(), consider propagating errors up to allow proper error handling by the caller.
daliuge-engine/dlg/apps/mpi.py
Outdated
| if not self._command: | ||
| raise InvalidDropException( | ||
| self, "No command specified, cannot create MPIApp" | ||
| ) | ||
| self._recompute_data = {} | ||
|
|
||
| def run(self): |
There was a problem hiding this comment.
issue (complexity): Consider breaking down the run() method into smaller helper methods for command building, redirection and environment setup
The run() method has grown complex with multiple string manipulation steps. Consider extracting the command building logic into helper methods while maintaining all functionality:
def _build_base_command(self, reader):
keyargs, pargs = replace_named_ports(
self._inputs.items(),
self._outputs.items(),
self.parameters.get("inputs", []),
self.parameters.get("outputs", []),
self._applicationArgs,
argumentPrefix=self._argumentPrefix,
separator=self._paramValueSeparator,
parser=reader,
)
args_str = f"{' '.join(map(str,pargs + keyargs))}"
return f"{self._command} {args_str} {self._args}".strip()
def _add_redirects(self, cmd):
if self._outputRedirect:
cmd = f"{cmd} > {self._outputRedirect}"
if self._inputRedirect:
cmd = f"cat {self._inputRedirect} > {cmd}"
return cmd.strip()
def _setup_environment(self):
env = os.environ.copy()
env.update({
"DLG_UID": self._uid,
"DLG_ROOT": utils.getDlgDir()
})
if self._dlg_session_id:
env.update({"DLG_SESSION_ID": self._dlg_session_id})
return envThis refactoring:
- Separates command construction into logical steps
- Makes the code easier to test and modify
- Keeps all functionality intact while reducing complexity
- Improves readability of the main run() method
There was a problem hiding this comment.
I agree with this AI recommendation, but suspect it is low priority. I think it might be a good backlog task.
| self.setpar("log_root", "LOG_DIR") | ||
| self.setpar("modules", "MODULES") | ||
| self.setpar("venv", "VENV") | ||
| self.setpar("exec_prefix", "EXEC_PREFIX") | ||
|
|
||
| @abstractmethod | ||
| def init_list(self): | ||
| pass | ||
|
|
||
| def setpar(self, k, v): |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the config value handling to separate template resolution from storage logic
The setpar() implementation adds unnecessary complexity by mixing dynamic attribute lookup with string templating. Consider simplifying by separating template resolution from value storage:
class DefaultConfig:
def __init__(self, user=None):
self._dict = {}
self._template_vars = {'USER': user} if user else {}
# Define base config with templates
self._dict.update({
'host': self.LOGIN_NODE,
'account': self.ACCOUNT,
'home_dir': self.HOME_DIR,
'dlg_root': self.DLG_ROOT,
'log_root': self.LOG_DIR,
'modules': self.MODULES,
'venv': self.VENV,
'exec_prefix': self.EXEC_PREFIX
})
# Resolve any templates
for k, v in self._dict.items():
if isinstance(v, str):
self._dict[k] = string.Template(v).safe_substitute(self._template_vars)This approach:
- Stores templates directly in config values
- Separates template variable definition from resolution
- Performs resolution in a single pass
- Removes complex attribute lookup logic
The code is more maintainable while preserving the user substitution functionality.
daliuge-engine/dlg/apps/mpi.py
Outdated
|
|
||
| # Pass down daliuge-specific information to the subprocesses as environment variables | ||
| env = os.environ.copy() | ||
| env.update({"DLG_UID": self._uid}) |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Add single value to dictionary directly rather than using update() [×3] (
simplify-dictionary-update) - Extract code out into method (
extract-method)
| config = ConfigFactory.mapping.get(facility)(user=user) | ||
| return config |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| config = ConfigFactory.mapping.get(facility)(user=user) | |
| return config | |
| return ConfigFactory.mapping.get(facility)(user=user) |
daliuge-engine/dlg/apps/mpi.py
Outdated
| if not self._command: | ||
| raise InvalidDropException( | ||
| self, "No command specified, cannot create MPIApp" | ||
| ) | ||
| self._recompute_data = {} | ||
|
|
||
| def run(self): |
There was a problem hiding this comment.
I agree with this AI recommendation, but suspect it is low priority. I think it might be a good backlog task.
|
I am merging this as the dlg_paletteGen failures are related to breaking changes in that project. |
Type
Problem/Issue
When executing a graph with a DockerApp calling a container with a pre-defined Entrypoint, any arguments were incorrectly passed on and the execution failed.
Solution
The problem was caused by the new addition of an entrypoint argument to the creation of the docker container. This is now only set if the associated entrypoint argument of the DockerApp is not None. In addition the command argument of the DockerApp can now be left empty and that will cause the default entrypoint to be used with the additional arguments passed in the Args list to the docker container.
Checklist
The description of the command argument of the DockerApp has been updated to guide the user.
Summary by Sourcery
Fix argument passing issue in DockerApp by conditionally setting the entrypoint. Enhance DockerApp to allow empty command arguments, using default entrypoints. Update deployment configurations to include user-specific settings.
Bug Fixes:
Enhancements:
Deployment: