Skip to content

56 no implicit benchname#60

Open
AymanBx wants to merge 3 commits intomainfrom
56-no-implicit-benchname
Open

56 no implicit benchname#60
AymanBx wants to merge 3 commits intomainfrom
56-no-implicit-benchname

Conversation

@AymanBx
Copy link
Contributor

@AymanBx AymanBx commented Feb 26, 2026

closing #56

@brownsarahm is this close to what you had in mind?

@AymanBx AymanBx requested a review from brownsarahm February 26, 2026 07:55
@AymanBx AymanBx linked an issue Feb 26, 2026 that may be closed by this pull request
Comment on lines +110 to +115
if os.path.isdir(benchmark_path):
content = os.listdir(benchmark_path)
if 'info.yml' in content:
benchmark = Bench.from_yaml(benchmark_path)
else:
benchmark = Bench.from_folders(benchmark_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be in a bench.load method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm confused because I don't see a bench.load method in benckmark.py

Copy link
Contributor

Choose a reason for hiding this comment

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

you could make one, this woudl be basically what it does

Comment on lines +117 to +118
if benchmark.written:
# Create Task object
Copy link
Contributor

Choose a reason for hiding this comment

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

how could this ever be false from the CLI usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this question, I don't think it would ever be false. But I thought to myself in case something goes wrong in the previous step and we magically reach here I'll put this check

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't do magic. I think the constructor would fail, so we don't need redundant logic.

Comment on lines +174 to +178
if benchmark.bench_name:
click.echo(f"Running {task_name} of benchmark {benchmark.bench_name} now")
benchmark.run_task(task_name, runner, log_path)
else:
click.echo(f"path {benchmark_path} doesn't seem to have a benchmark in it")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the error would be caught by the constructor, wouldn't it

Comment on lines +207 to +211
if benchmark.bench_name:
click.echo(f"Running {benchmark.bench_name} now")
benchmark.run(runner, log_path)
else:
click.echo(f"path {benchmark_path} doesn't seem to have a benchmark in it")
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Co-authored-by: Sarah Brown <brownsarahm@uri.edu>
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.

no implicit benchname

2 participants