[WIP] Add Fireworks backend - #1948
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a dedicated Fireworks Training API backend for SkyRL, implementing both synchronous and fully-asynchronous GRPO. It adds the necessary runtime, inference, and training dispatch adapters, along with configuration validation, scripts, and unit tests. The feedback highlights a few areas for improvement: avoiding the hardcoded absolute local path for the harbor dependency in pyproject.toml to ensure portability, refactoring the fragile access to the private _managed_handle attribute in the Fireworks runtime, and safely converting PyTorch tensors to Python lists using .tolist() in the inference client to prevent type mismatches.
| # Absolute because Ray stages this project in a temporary working directory; | ||
| # a ../harbor-private relative source would then resolve inside Ray's staging | ||
| # tree instead of to the editable checkout on this pinned single node. | ||
| harbor = { path = "/home/ray/default/harbor-private", editable = true } |
There was a problem hiding this comment.
Hardcoding an absolute local path (/home/ray/default/harbor-private) in pyproject.toml makes the project non-portable and will break installation for other developers or in CI/CD environments. Consider using a relative path if the dependency is always in the same parent directory, or keep the git dependency in the committed configuration and use local overrides (e.g., uv pip install -e) during development.
| def inference_endpoint(self) -> FireworksInferenceEndpoint: | ||
| """Return the native endpoint backing the managed rollout deployment.""" | ||
|
|
||
| handle = getattr(self.service, "_managed_handle", None) |
There was a problem hiding this comment.
Accessing the private attribute _managed_handle of self.service is fragile as private attributes (prefixed with an underscore) are not part of the public API and can be changed or removed in future versions of the fireworks-ai SDK without notice. If a public API or property exists to retrieve the inference model and URL, please use it instead. Otherwise, consider adding a comment explaining why this private access is necessary and handle potential AttributeErrors gracefully.
| self._sample_one( | ||
| list(tokens), params=params, request_logprobs=request_logprobs | ||
| ) |
There was a problem hiding this comment.
If prompt_token_ids contains PyTorch tensors (especially on GPU), calling list(tokens) will produce a list of PyTorch scalar tensors rather than standard Python ints. This can cause type mismatches or device errors when passed to tinker.ModelInput.from_ints. It is safer to convert the tokens using .tolist() if it is a tensor, or explicitly cast the elements to integers.
self._sample_one(
tokens.tolist() if hasattr(tokens, "tolist") else list(tokens),
params=params,
request_logprobs=request_logprobs,
)1 participant
No description provided.