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

[Part 1] Display logs for client and server #128

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

[Part 1] Display logs for client and server #128

wants to merge 11 commits into from

Conversation

lotif
Copy link
Collaborator

@lotif lotif commented Nov 25, 2024

PR Type

Feature

Short Description

Clickup Ticket(s): https://app.clickup.com/t/8689qq767

I'm splitting this ticket into 2 PRs: one for back end and one for front end.

This is the back end one, where I am making 2 endpoints to retrieve the logs for the server and the clients of a job.

In order to make it possible, I had to add the location of the log paths in the filesystem to the database. The log file paths had to be returned from the functions that start both clients and servers, and also the client endpoint that starts a client. That approach may not work when we move this to slurm jobs, but we'll cross that bridge when we get there.

Tests Added

Fully unit and integration tested

florist/api/client.py Dismissed Show dismissed Hide dismissed
florist/api/routes/server/job.py Dismissed Show dismissed Hide dismissed
florist/api/routes/server/job.py Dismissed Show dismissed Hide dismissed
florist/api/routes/server/job.py Dismissed Show dismissed Hide dismissed
@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 93.10345% with 4 lines in your changes missing coverage. Please review.

Project coverage is 93.95%. Comparing base (968df3b) to head (a1b7b66).

Files with missing lines Patch % Lines
florist/api/routes/server/job.py 86.66% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #128      +/-   ##
==========================================
- Coverage   93.99%   93.95%   -0.05%     
==========================================
  Files          24       24              
  Lines        2166     2217      +51     
  Branches      178      120      -58     
==========================================
+ Hits         2036     2083      +47     
- Misses        130      134       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

0 <= client_index < len(job.clients_info)
), f"Client index {client_index} is invalid (total: {len(job.clients_info)})"

client_info = job.clients_info[client_index]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is almost certainly a naive question, but is there a reason we are relying on the server to tell the clients where their log files are? In my head, rather than passing the location of the log to the client, the client will have stored that path already and you would just hit it with a get call to tell it that you want its log file. There's likely something more complicated there that I'm not seeing though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I sort of see where the complexity is coming from. Basically you want the server to set the log location for each client. Perhaps that's something we can communicate on startup and then don't have to worry about the server managing that from there on out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The thing here is we need to store that log path somewhere. The server already has a connection to a database, along with all the other information from the clients, so I thought it would be easier to store it there.

On startup, the client will pass to the server the location of its logs, who will in turn save it on the database. The log location only lives in memory for the client, which is lost after the flower client is started up, so the server is responsible for permanently storing it for later retrieval.

Let me know if that makes sense.

Copy link
Collaborator

@emersodb emersodb left a comment

Choose a reason for hiding this comment

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

Overall, I think this looks good to me. Really extensive tests!

Had one comment of any substance, but I'm likely just missing something there.

lotif and others added 2 commits December 9, 2024 15:08
# Conflicts:
#	florist/tests/unit/api/routes/server/test_training.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants