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.

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

Can't run solve() inside a ThreadPool with GLPK #2475

Closed
ldeluigi opened this issue Aug 1, 2022 · 14 comments · Fixed by #2514
Closed

Can't run solve() inside a ThreadPool with GLPK #2475

ldeluigi opened this issue Aug 1, 2022 · 14 comments · Fixed by #2514

Comments

@ldeluigi
Copy link
Contributor

ldeluigi commented Aug 1, 2022

Summary

From https://stackoverflow.com/questions/73064859/pyomo-how-do-i-run-multiple-solve-concurrently-in-parallel-on-the-same-model:

I'm using opt = pyo.SolverFactory('glpk'), and I'd like to call opt.solve(), multiple times, inside a map function of a ThreadPool, but it gives error caused by race conditions.

It doesn't work even if I instanciate opt inside the map function.

The errors I get are random, but in many cases they refer to a log_file which can't be opened.

I'm even cloning the model for each map function call, to avoid race conditions on it.

I can't use MPI, I need to use a ThreadPool (which parallelizes only I/O ops, like waiting for the solver to end the task, which is good enough for me)

Steps to reproduce the issue

import pyomo.environ as pyo
from pyomo.opt import SolverFactory
from multiprocessing.dummy import Pool as ThreadPool

model = pyo.ConcreteModel()
model.nVars = pyo.Param(initialize=4)
model.N = pyo.RangeSet(model.nVars)
model.x = pyo.Var(model.N, within=pyo.Binary)
model.obj = pyo.Objective(expr=pyo.summation(model.x))
model.cuts = pyo.ConstraintList()

def test(_):
    opt = SolverFactory('glpk')
    opt.solve(model) 

    # Iterate, adding a cut to exclude the previously found solution
    for i in range(5):
        expr = 0
        for j in model.x:
            if pyo.value(model.x[j]) < 0.5:
                expr += model.x[j]
            else:
                expr += (1 - model.x[j])
        model.cuts.add( expr >= 1 )
        results = opt.solve(model)
        return results

tp = ThreadPool(4)
results = tp.map(test, range(5))
print(results)

or:

import pyomo.environ as pyo
from pyomo.opt import SolverFactory
from multiprocessing.dummy import Pool as ThreadPool

model = pyo.ConcreteModel()
model.nVars = pyo.Param(initialize=4)
model.N = pyo.RangeSet(model.nVars)
model.x = pyo.Var(model.N, within=pyo.Binary)
model.obj = pyo.Objective(expr=pyo.summation(model.x))
model.cuts = pyo.ConstraintList()

def test(model):
    opt = SolverFactory('glpk')
    opt.solve(model) 

    # Iterate, adding a cut to exclude the previously found solution
    for i in range(5):
        expr = 0
        for j in model.x:
            if pyo.value(model.x[j]) < 0.5:
                expr += model.x[j]
            else:
                expr += (1 - model.x[j])
        model.cuts.add( expr >= 1 )
        results = opt.solve(model)
        return results

tp = ThreadPool(4)
results = tp.map(test, [model.clone() for i in range(4)])
print(results)

Error Message

WARNING: Empty constraint block written in LP format - solver may error
WARNING: Empty constraint block written in LP format - solver may error
WARNING: Empty constraint block written in LP format - solver may error
WARNING: Empty constraint block written in LP format - solver may error
Traceback (most recent call last):
  File "c:\Users\_\Desktop\problem.py", line 29, in <module>
    results = tp.map(test, [model.clone() for i in range(4)])
  File "D:\Programs\Python310\lib\multiprocessing\pool.py", line 364, in map
    return self._map_async(func, iterable, mapstar, chunksize).get()
  File "D:\Programs\Python310\lib\multiprocessing\pool.py", line 771, in get
    raise self._value
  File "D:\Programs\Python310\lib\multiprocessing\pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
  File "D:\Programs\Python310\lib\multiprocessing\pool.py", line 48, in mapstar
    return list(map(*args))
  File "c:\Users\_\Desktop\problem.py", line 14, in test
    opt.solve(model)
  File "D:\Programs\Python310\lib\site-packages\pyomo\opt\base\solvers.py", line 602, in solve
    result = self._postsolve()
  File "D:\Programs\Python310\lib\site-packages\pyomo\opt\solver\shellcmd.py", line 281, in _postsolve
    results = self.process_output(self._rc)
  File "D:\Programs\Python310\lib\site-packages\pyomo\opt\solver\shellcmd.py", line 354, in process_output
    results = self.process_logfile()
  File "D:\Programs\Python310\lib\site-packages\pyomo\solvers\plugins\solvers\GLPK.py", line 213, in process_logfile
    with open(self._log_file, 'r') as output:
OSError: [WinError 6] The handle is invalid
Exception ignored in: <function TeeStream.__del__ at 0x00000237C62AE830>
Traceback (most recent call last):
  File "D:\Programs\Python310\lib\site-packages\pyomo\common\tee.py", line 425, in __del__
    self.close()
  File "D:\Programs\Python310\lib\site-packages\pyomo\common\tee.py", line 407, in close
    h.finalize(self.ostreams)
  File "D:\Programs\Python310\lib\site-packages\pyomo\common\tee.py", line 267, in finalize
    os.close(self.read_pipe)
OSError: [Errno 9] Bad file descriptor

Information on your system

Pyomo version: 6.4.1
Python version: 3.10.5
Operating system: Windows
How Pyomo was installed (PyPI, conda, source): PyPI
Solver (if applicable): GLPK

Additional information

I posted on the forum too, but none responded:
https://groups.google.com/g/pyomo-forum/c/CovfenBC0wg

@ldeluigi
Copy link
Contributor Author

ldeluigi commented Aug 7, 2022

After some investigation, I found out that the problem is related to the fact that each solve() call uses the same variable to store the log file name, because there is only one singleton TempFileManager instance.

This issue could be solved by avoiding using one singleton for all the temporary files, allowing to pass a tempfilemanager instance either to solve or solverfactory, making them efectively independent (i.e. thread safe).

@jsiirola
Copy link
Member

jsiirola commented Aug 9, 2022

Thanks for the bug report and the follow-up. The root of this is that the TempfileManager was originally written to be used as a stack (which is inherently not thread safe in the way you want to use it). We have updated it so that it can be used in threaded environments. Unfortunately, the "stack" usage is hard coded into the older solver interfaces, and a design decision from the very beginning of Pyomo (2008) makes it very difficult to replace the "stack" usage of the TempfileManager with the thread-safe explicit management of the temp file context. This is on the radar to fix, but it will take some time.

An alternative (more expedient) solution may be to extend the APPSI interface to support GLPK. @michaelbynum?

@ldeluigi
Copy link
Contributor Author

I thought it would just be a matter of removing the global instance (the singleton) and replacing broken references with new references to a local instance, optionally passed to the constructor of the solver factory maybe

@ldeluigi
Copy link
Contributor Author

Each factory would then use its own stack I think. Maybe I didn't understand something. If that's the case, I'm sorry

@jsiirola
Copy link
Member

The problem is that for the older solver interfaces the Solver doesn't directly create / manage the problem writer (there is a sort of "meet in the middle" infrastructure - dating back to the Coopr project when the solver interfaces were not explicitly tied to the Pyomo modeling environment). This makes it very challenging for the solver to inform the writer of which TempfileManager instance / context that it should use. Newer solvers (e.g., APPSI) explicitly handle the problem writers, so things could work closer to what you suggest.

@ldeluigi
Copy link
Contributor Author

Another problem would be that there is no APPSI for GLPK

@michaelbynum
Copy link
Contributor

Another problem would be that there is no APPSI for GLPK

There will be, but it will take some time.

@ldeluigi
Copy link
Contributor Author

Alright, thanks.

@ldeluigi
Copy link
Contributor Author

ldeluigi commented Aug 25, 2022

Today I came up with a monkeypatch idea for the problem.
What do you think about this code? I've put it at the bottom of tempfiles.py locally.
I tested it on my machine (Windows, python 3.10.5) and seems to work in my use case.

from threading import get_ident
class MultiThreadTempFileManager():
    def __init__(self):
        self.tfm_dict = {}

    def __getattr__(self, attr):
        id = get_ident()
        if id not in self.tfm_dict:
            self.tfm_dict[id] = self.__new_manager(id)
        return getattr(self.tfm_dict[id], attr)

    def __new_manager(self, id):
        t = TempfileManagerClass()
        t.tempdir = os.path.join(tempfile.gettempdir(), 'pyomo_' + str(id))
        if os.path.isdir(t.tempdir):
            shutil.rmtree(t.tempdir)
        elif os.path.isfile(t.tempdir):
            os.remove(t.tempdir)
        os.makedirs(t.tempdir, exist_ok=False)
        return t

# The global Pyomo TempfileManager instance
TempfileManager = MultiThreadTempFileManager()

If you approve, I can open a PR.

[Edit]
the solution can probably be improved to ensure that tempdir is deleted afterwards.

@jsiirola
Copy link
Member

@ldeluigi: Thank you! I think this approach has promise, and a PR would be great.

Some immediate thoughts:

  • I like the idea of making the TempfileManager "thread-specific" (we probably want to also consider this in other contexts where we have global stacks, e.g., PauseGC)
  • I don't think that each thread-specific TempfileManager needs to create their own directory. As each TempfileManager will maintain a list of the tempfiles that it creates (and it uses the underlying system tools for ensuring that tempfiles don't collide), there shouldn't be an issue with collisions or cleanup.
  • When we create new TempfileManager instances (e.g., for new threads), we should copy over / inherit the tempdir from the main thread. Alternatively, _resolve_tempdir() should look to the main thread as part of the search path [I think I like this approach better]
  • The MultiThreadedTempfileManager should have an API to access the manager associated with the "main" thread.

@ldeluigi
Copy link
Contributor Author

I'll work on this in the future, thanks for the feedback!

@ldeluigi
Copy link
Contributor Author

  • I like the idea of making the TempfileManager "thread-specific" (we probably want to also consider this in other contexts where we have global stacks, e.g., PauseGC)

Yeah, PauseGC is the source of similar errors. Are there any other examples of global stacks that need work for thread safety?

@jsiirola
Copy link
Member

Good question. The short answer is "yes". The longer answer is "but I don't have a definitive list." We would have to go and look for module-scope variables and singleton-like things. I know that there is one in the new NLwriter. There are likely more.

@ldeluigi
Copy link
Contributor Author

Unfortunately I have to leave those one out because of time availability. I'm focusing on PauseGC and TempFileManager.

I'll open a PR shortly so that you can review my code and help improve it. I'll link it to this issue.

blnicho added a commit that referenced this issue Sep 13, 2022
Fix #2475 - Implement "thread safe" proxies for singleton instances (PauseGC, TempFileManager)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants