Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[core] Use statusor for filesystem and resolve dependency #50029

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dentiny
Copy link
Contributor

@dentiny dentiny commented Jan 23, 2025

Address @jjyao 's comment to use StatusOr to read all content for the given file.
Also cleanup dependency for ray utils, otherwise we have circular dependency among status, logging, filesystem and utils.

@dentiny dentiny added the go add ONLY when ready to merge, run all tests label Jan 23, 2025
@dentiny dentiny requested review from jjyao and rynewang January 23, 2025 05:00
@dentiny dentiny force-pushed the hjiang/filesystem-statusor branch from 15d7c05 to f51d822 Compare January 23, 2025 05:49
@dentiny dentiny force-pushed the hjiang/filesystem-statusor branch from f51d822 to 2d55e3c Compare January 23, 2025 07:42
Signed-off-by: dentiny <[email protected]>
@@ -367,7 +367,7 @@ void RayLog::InitLogFormat() {
app_name_without_path = "DefaultApp";
} else {
// Find the app name without the path.
std::string app_file_name = ray::GetFileName(app_name);
std::string app_file_name = std::filesystem::path(app_name).filename().string();
Copy link
Contributor Author

@dentiny dentiny Jan 23, 2025

Choose a reason for hiding this comment

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

To reduce circular dependency between logging and filesystem.

Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
@dentiny dentiny requested a review from rynewang January 24, 2025 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go add ONLY when ready to merge, run all tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants