Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion trinity/explorer/explorer.py
Original file line number Diff line number Diff line change
Expand Up @@ -494,13 +494,14 @@ async def serve(self) -> None:
self.service.set_latest_model_version(step_num)

@classmethod
def get_actor(cls, config: Config):
def get_actor(cls, config: Config, runtime_env: Optional[dict] = None):
"""Get a Ray actor for the explorer."""
return (
ray.remote(cls)
.options(
name=config.explorer.name,
namespace=ray.get_runtime_context().namespace,
runtime_env=runtime_env,
)
.remote(config)
)
Comment on lines +497 to 507
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The change to support runtime_env is a good addition. However, the new functionality is not covered by tests. To ensure its correctness and prevent future regressions, please consider adding a test case that verifies the runtime_env is correctly passed to the Ray actor. For example, you could pass an environment variable and check its value inside the actor.

10 changes: 7 additions & 3 deletions trinity/trainer/trainer.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import time
import traceback
from abc import ABC, abstractmethod
from typing import Dict, List, Tuple
from typing import Dict, List, Optional, Tuple

import pandas as pd
import ray
Expand Down Expand Up @@ -216,11 +216,15 @@ async def is_alive(self) -> bool:
return True

@classmethod
def get_actor(cls, config: Config):
def get_actor(cls, config: Config, runtime_env: Optional[dict] = None):
"""Get a Ray actor for the trainer."""
return (
ray.remote(cls)
.options(name=config.trainer.name, namespace=ray.get_runtime_context().namespace)
.options(
name=config.trainer.name,
namespace=ray.get_runtime_context().namespace,
runtime_env=runtime_env,
)
.remote(config)
)
Comment on lines +219 to 229
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to the explorer, this change to support runtime_env for the trainer actor is useful. To improve the robustness of the code, it would be beneficial to add a test case to ensure it works as expected.


Expand Down