Skip to content

Conversation

@anandhu-eng
Copy link
Contributor

No description provided.

@anandhu-eng anandhu-eng requested a review from a team as a code owner November 19, 2024 10:17
@anandhu-eng anandhu-eng marked this pull request as draft November 19, 2024 10:17
@github-actions
Copy link

github-actions bot commented Nov 19, 2024

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅


user_conf_path = os.path.join(result_scenario_path, "user.conf")

measurements_json_path = os.path.join(result_scenario_path, "measurements.json")
Copy link
Contributor

Choose a reason for hiding this comment

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

It is better to move this code to near line number 422 to avoid redundancy.

shutil.copy(measurements_json_path, os.path.join(target_measurement_json_path, sub_res+'.json'))
shutil.copy(measurements_json_path, os.path.join(target_measurement_json_path, 'model-info.json'))
else:
if not mode.startswith("TEST"):
Copy link
Contributor

Choose a reason for hiding this comment

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

we should error out only in performance mode right?

# This line can be removed once the PR in the inference repo is merged.
shutil.copy(measurements_json_path, os.path.join(submission_measurement_path, sub_res+'.json'))
shutil.copy(measurements_json_path, os.path.join(submission_measurement_path, 'model-info.json'))
shutil.copy(measurements_json_path, os.path.join(target_measurement_json_path, sub_res+'.json'))
Copy link
Contributor

Choose a reason for hiding this comment

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

PR is merged. So, we can remove this line

if not all([os.path.exists(os.path.join(result_scenario_path, "performance", "run_1", f)) for f in files_to_check]):
continue

if not os.path.isdir(measurement_scenario_path):
Copy link
Contributor

Choose a reason for hiding this comment

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

needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @arjunsuresh , i have placed it above the for loop as submission_measurement_path and measurement_scenario_path are the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

okay 👍

tags: _short-run
env:
CM_MLPERF_SUBMISSION_DIVISION: open
CM_RUN_MLPERF_SUBMISSION_PREPROCESSOR: off
Copy link
Contributor

Choose a reason for hiding this comment

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

Theres already an env in this variation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

missed due to space in between... fixed:)

@anandhu-eng anandhu-eng marked this pull request as ready for review November 19, 2024 15:16
@anandhu-eng anandhu-eng marked this pull request as draft November 19, 2024 15:16
@anandhu-eng anandhu-eng marked this pull request as ready for review November 19, 2024 18:35
@arjunsuresh arjunsuresh merged commit 6638a79 into mlperf-inference Nov 21, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 2024
@arjunsuresh arjunsuresh deleted the issue-#542 branch November 24, 2024 00:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants