Skip to content

Conversation

@Yadunund
Copy link
Collaborator

@Yadunund Yadunund commented Feb 9, 2025

This PR updates the tester to compile results into a csv file in a format similar to what was expected in previous bop challenges. The only difference being instead of separating matrix elements by spaces, we use commas.

This file is further written to disk to directory / filename that can be configured. By default it writes to /submission/submission.cvs. README is updated to mount another volume for this output directory so that the results can be accessed outside of the container.

Lastly, I took the liberty to bump the zenoh router to 1.2.1. to match the zenoh version in the latest jazzy binaries that were released today

@Yadunund Yadunund requested review from tfoote and vaheta February 11, 2025 04:30
# result stage: base + copied install folders from the overlay + service setup.
FROM base

ARG DATASET_NAME
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be set to default as "ipd"?

Suggested change
ARG DATASET_NAME
ARG DATASET_NAME=ipd

Copy link
Collaborator

Choose a reason for hiding this comment

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

Embedding these into the image is causing a challenge as it gets baked into the command as well. In working on the automation I have been manually overriding the DATASET_NAME and building the image locally to get it working quickly instead of trying to override the full CMD. I think that the CMD is getting baked with the build ENV, not the runtime ENV.

Copy link
Collaborator

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

I've said this before I'd really like to get any of the runtime settings out of the image so that they're reusable and we don't have to rebuild them to change what dataset we're running against. Lets follow up on that separately though..

I added a commit extending bpc command line tool to support mounting it through.

# result stage: base + copied install folders from the overlay + service setup.
FROM base

ARG DATASET_NAME
Copy link
Collaborator

Choose a reason for hiding this comment

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

Embedding these into the image is causing a challenge as it gets baked into the command as well. In working on the automation I have been manually overriding the DATASET_NAME and building the image locally to get it working quickly instead of trying to override the full CMD. I think that the CMD is getting baked with the build ENV, not the runtime ENV.

test_parser.add_argument("estimator_image")
test_parser.add_argument("dataset")
test_parser.add_argument("--dataset_directory", action="store", default=".")
test_parser.add_argument("--result_directory", action="store", default=".")
Copy link
Collaborator

Choose a reason for hiding this comment

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

bpc will now offer to set this and automatically mount it through for you.

@Yadunund
Copy link
Collaborator Author

I've said this before I'd really like to get any of the runtime settings out of the image so that they're reusable and we don't have to rebuild them to change what dataset we're running against. Lets follow up on that separately though..

Sounds good.

Also a little bit of cleanup of command line parsing.

Signed-off-by: Tully Foote <[email protected]>
Signed-off-by: Yadunund <[email protected]>
@veb-101
Copy link
Contributor

veb-101 commented Feb 11, 2025

Can you guys also update either the build or run command for the tester container to pass the DATASET_NAME in the README.md and mention the required dataset directory structure?

@tfoote
Copy link
Collaborator

tfoote commented Feb 11, 2025

yeah, I have defaulted it as well in #35 for now.

@tfoote
Copy link
Collaborator

tfoote commented Feb 11, 2025

I'm going to merge this as the build is broken without it. e.g #36

@tfoote tfoote merged commit 7acf0c4 into main Feb 11, 2025
2 checks passed
@Yadunund Yadunund deleted the vt/tester_update branch February 11, 2025 19:23
AllProAi pushed a commit to AllProAi/bpc that referenced this pull request Mar 17, 2025
ramamoorthyluxman pushed a commit to ramamoorthyluxman/bpc that referenced this pull request Aug 5, 2025
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.

5 participants