fix: validate launcher service env before PTY launch#44
fix: validate launcher service env before PTY launch#44haosenwang1018 wants to merge 2 commits intomodelscope:mainfrom
Conversation
The LlmDedupSamplingExploreStrategy class was using EnvWorkerWithPrompt which doesn't exist. Based on issue modelscope#31 and comparison with the similar LlmRandomSamplingExploreStrategy, this should use EnvWorker instead. Changes: - Import EnvWorker and TrajExpConfig - Replace EnvWorkerWithPrompt instantiation with EnvWorker using correct interface (task, config, thread_index, tokenizer) - Update execute() call to include required parameters (traj_exp_config, tmux, stop) matching the pattern used in random strategy Fixes modelscope#31
There was a problem hiding this comment.
Code Review
This pull request refactors the LlmDedupExploreStrategy to use EnvWorker instead of EnvWorkerWithPrompt, updating the execution parameters to include trajectory configuration and execution controls. It also introduces a robust environment variable validation helper in launcher.py along with new unit tests. A regression was identified in the deduplication strategy where the environment service URL is no longer explicitly passed to the worker, which may ignore custom configuration overrides provided during initialization.
| env_worker = EnvWorker( | ||
| task=task, | ||
| config=self._config, | ||
| thread_index=0, | ||
| tokenizer=self._tokenizer, | ||
| ) |
There was a problem hiding this comment.
The transition from EnvWorkerWithPrompt to EnvWorker ignores self._env_service_url. Previously, this URL was explicitly passed to the worker (line 60 on the LEFT side). Now, EnvWorker relies on self._config.env_service.env_url internally, which means any custom environment URL provided during the strategy's initialization (via kwargs) is ignored. This is a regression in functionality that prevents overriding the environment service URL for this specific strategy.
Summary
*_PATHor*_SCRIPTenv vars are missing for PTY-managed servicesLaunchCommandWhenAbsentTesting
Closes #9