Skip to content
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.

Sign up for GitHub

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

Jump to bottom

The user can modify NG_CPUS_PER_TASK #1336

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

The user can modify NG_CPUS_PER_TASK #1336

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c4d7a78
The user can modify NG_CPUS_PER_TASK
teytaud Jan 19, 2022
276842c
Update xpbase.py
teytaud Jan 19, 2022
b54085a
Update xpbase.py
teytaud Jan 19, 2022
2ca61e5
fix
teytaud Jan 20, 2022
d03868b
fix
teytaud Jan 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions nevergrad/benchmark/xpbase.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@
import time
import random
import numbers
import os
import warnings
import traceback
import typing as tp
import numpy as np
from concurrent import futures
from nevergrad.parametrization import parameter as p
from nevergrad.common import decorators
from nevergrad.common import errors
Expand Down Expand Up @@ -57,15 +59,20 @@ def __init__(
self.optimizer = optimizer
self.budget = budget
self.num_workers = num_workers
self.executor = execution.MockedTimedExecutor(batch_mode)
ng_cpus_per_task = int(os.getenv("NG_CPUS_PER_TASK", "1"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is only for benchmark so let's make it clear

Suggested change
ng_cpus_per_task = int(os.getenv("NG_CPUS_PER_TASK", "1"))
ng_cpus_per_task = int(os.getenv("NG_BENCHMARK_CPUS_PER_TASK", "1"))

if ng_cpus_per_task > 1:
self.executor: tp.Any = futures.ThreadPoolExecutor(max_workers=ng_cpus_per_task)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still very much against this since it

  • prevents reproducibility
  • adds complexity which may create a lot of issues (eg: not sure that it will checkpoint correctly if preempted)
  • assumes the benchmarks are threadsafe, which is not always true on our side and possibly on the dependency side

Can you quantify the speed gain?
Python executes one thread at a time for most operations, so it may just waste time for creating threads without going faster (depending on what is happening in the underlying code)
Moving to a ProcessPoolExecutor could help for parallelization, but I would expect a big overhead so that it could be unhelpful most of the time

assert not batch_mode
else:
self.executor = execution.MockedTimedExecutor(batch_mode)

@property
def name(self) -> str:
return self.optimizer if isinstance(self.optimizer, str) else repr(self.optimizer)

@property
def batch_mode(self) -> bool:
return self.executor.batch_mode
return hasattr(self.executor, "batch_mode") and self.executor.batch_mode
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return hasattr(self.executor, "batch_mode") and self.executor.batch_mode
return isinstance(self.executor, execution.MockedTimedExecutor) and self.executor.batch_mode

Copy link
Contributor

Choose a reason for hiding this comment

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

checking hasattr is not safe, if we update the code that could break without notice, it's much safer to test against a class


def __repr__(self) -> str:
return f"Experiment: {self.name}<budget={self.budget}, num_workers={self.num_workers}, batch_mode={self.batch_mode}>"
Expand Down Expand Up @@ -207,7 +214,9 @@ def run(self) -> tp.Dict[str, tp.Any]:
def _log_results(self, pfunc: fbase.ExperimentFunction, t0: float, num_calls: int) -> None:
"""Internal method for logging results before handling the error"""
self.result["elapsed_time"] = time.time() - t0
self.result["pseudotime"] = self.optimsettings.executor.time
self.result["pseudotime"] = (
self.optimsettings.executor.time if hasattr(self.optimsettings.executor, "time") else float("nan")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.optimsettings.executor.time if hasattr(self.optimsettings.executor, "time") else float("nan")
self.optimsettings.executor.time if isinstance(self.executor, execution.MockedTimedExecutor) else float("nan")

)
# make a final evaluation with oracle (no noise, but function may still be stochastic)
opt = self._optimizer
assert opt is not None
Expand Down