-
Notifications
You must be signed in to change notification settings - Fork 12
Parallel benchmarks #176
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
base: main
Are you sure you want to change the base?
Parallel benchmarks #176
Conversation
@dpo I rebased on the your changes and cleaned up the git pushes |
@@ -13,6 +14,7 @@ Apply a solver to a set of problems. | |||
CUTEst problems). | |||
|
|||
#### Keyword arguments | |||
* `use_threads::Bool`: whether to use threads (default: `true`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this keyword? Threads should be enabled/disabled with the environment variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the use_threads
keyword is valuable to keep, even with the environment variable, for the following reasons:
Testing and Debugging: This flag allows us to explicitly override the environment variable during testing or debugging. It’s particularly useful to verify that the application behaves correctly in both threaded and non-threaded modes without re-writing the functionality in the unit tests.
Fine-Grained User Control: Some users may want to override the global threading setting for specific scenarios:
They might want to disable threading for this application while leaving it enabled for other programs to avoid resource contention.
I think keeping this flag ensures flexibility and adaptability, making the tool more useful for both development and usage scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A user who doesn’t set JULIA_NUM_THREADS
is not expected anything to run multi-threaded, yet use_threads
will be true
. I think that only introduces confusion.
prune::Bool = true, | ||
kwargs..., | ||
) where {TName} | ||
|
||
# Collect information about counters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove these comments and blank lines that do not have anything to do with the new functionality.
@info log_header(colstats, types[col_idx], hdr_override = info_hdr_override) | ||
|
||
for (id, problem) in enumerate(problems) | ||
# Convert problems to an indexable vector | ||
problem_list = collect(problems) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We really do not want this. If problems
is a generator of all CUTEst models, this line will try to materialize them all at once and hold them all in memory simultaneously, which is not acceptable.
Cleaned up the code and rebased on newest changes from #167