Skip to content

Commit

Permalink
Fixing bugs found while preparing for the in demo (#69)
Browse files Browse the repository at this point in the history
    Saving a new job would format server config inappropriately. Fixing it to the proper format before sending data to be saved.
    Numeric values for server config were being submitted as strings, which breaks Flower clients. Fixing it by automatically converting numeric strings into numbers.
    Adding a loading state to the start training button so we have a visual indication that the call is still running.
  • Loading branch information
lotif authored Jun 27, 2024
1 parent b7092dc commit 3064482
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 6 deletions.
10 changes: 9 additions & 1 deletion florist/api/servers/config_parsers.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
"""Parsers for FL server configurations."""

import json
from ast import literal_eval
from contextlib import suppress
from enum import Enum
from typing import Any, Dict, List

Expand All @@ -26,7 +28,13 @@ def parse(cls, config_json_str: str) -> Dict[str, Any]:
:return: (Dict[str, Any]) The configuration JSON string parsed as a dictionary.
"""
config = json.loads(config_json_str)
assert isinstance(config, dict)
assert isinstance(config, dict), "config is not a dictionary"

for config_name in config:
# converting the value to number if it is a number
# if it throws an exception it means it's not a number, so suppress and leave as is
with suppress(Exception):
config[config_name] = literal_eval(config[config_name])

mandatory_fields = cls.mandatory_fields()

Expand Down
10 changes: 9 additions & 1 deletion florist/app/jobs/edit/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@ export function makeEmptyClientInfo() {
};
}

function formatServerConfig(serverConfig: Array<ServerConfig>): string {
const serverConfigDict = {};
for (let sc of serverConfig) {
serverConfigDict[sc.name] = sc.value;
}
return JSON.stringify(serverConfigDict);
}

export default function EditJob(): ReactElement {
return (
<div className="col-7 align-items-center">
Expand Down Expand Up @@ -94,7 +102,7 @@ export function EditJobForm(): ReactElement {

const job = { ...state.job };
// Server config is a json string, so changing it here before sending the data over
job.server_config = JSON.stringify(job.server_config);
job.server_config = formatServerConfig(job.server_config);
await post("/api/server/job", JSON.stringify(job));
}

Expand Down
17 changes: 15 additions & 2 deletions florist/app/jobs/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -111,15 +111,28 @@ export function StartJobButton({ rowId, jobId }: { rowId: number; jobId: string
// Only refresh the job data if there is an error or response
useEffect(() => refreshJobsByJobStatus(Object.keys(validStatuses)), [error, response]);

let buttonClasses = "btn btn-sm mb-0 ";
if (isLoading || response) {
// If is loading or if a successful response has been received,
// disable the button to avoid double submit.
buttonClasses += "btn-secondary disabled";
} else {
buttonClasses += "btn-primary";
}

return (
<div>
<button
data-testid={`start-training-button-${rowId}`}
onClick={handleClickStartJobButton}
className="btn btn-primary btn-sm mb-0"
className={buttonClasses}
title="Start"
>
<i className="material-icons text-sm">play_circle_outline</i>
{isLoading || response ? (
<span class="spinner-border spinner-border-sm align-middle"></span>
) : (
<i className="material-icons text-sm">play_circle_outline</i>
)}
</button>
</div>
);
Expand Down
7 changes: 5 additions & 2 deletions florist/tests/unit/app/jobs/edit/page.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,10 @@ describe("New Job Page", () => {
const submitButton = container.querySelector("button#job-post");
await act(async () => await submitButton.click());

testJob.server_config = JSON.stringify(testJob.server_config);
testJob.server_config = JSON.stringify({
[testJob.server_config[0].name]: testJob.server_config[0].value,
[testJob.server_config[1].name]: testJob.server_config[1].value,
});
expect(postMock).toBeCalledWith("/api/server/job", JSON.stringify(testJob));
});

Expand All @@ -370,7 +373,7 @@ describe("New Job Page", () => {
await act(async () => await submitButton.click());

const testJob = makeEmptyJob();
testJob.server_config = JSON.stringify(testJob.server_config);
testJob.server_config = JSON.stringify({ "": "" });
expect(postMock).toBeCalledWith("/api/server/job", JSON.stringify(testJob));

const errorAlert = container.querySelector("div#job-save-error");
Expand Down

0 comments on commit 3064482

Please sign in to comment.