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

equilibrium: dask scheduler default incorrectly set to synchronous #101

Open
richardotis opened this Issue Jun 16, 2017 · 10 comments

Comments

Projects
None yet
2 participants
@richardotis
Collaborator

richardotis commented Jun 16, 2017

The default setting of the scheduler kwarg in equilibrium is dask.async.get_sync, which means that the computation is being performed sequentially on a single core. This is suboptimal for any line/map calculation.

It looks like that change slipped in about 9 months ago in an unrelated commit, which is one of many reasons why we now do code review.

@bocklund

This comment has been minimized.

Collaborator

bocklund commented Jun 16, 2017

Is multiprocessing enough or should we set up a client by default?

@richardotis

This comment has been minimized.

Collaborator

richardotis commented Jun 16, 2017

I think scheduler should be dask.multiprocessing.get by default, but if the number of calculations is "small", we should just fall back to dask.async.get_sync (or whatever it's called now in newer versions).

@richardotis

This comment has been minimized.

Collaborator

richardotis commented Jun 16, 2017

We compute the number of calculations toward the bottom of equilibrium()

@bocklund

This comment has been minimized.

Collaborator

bocklund commented Jul 25, 2017

Some examples from NIMS databases. I assumed that parallelization in dask was independent of the shape of the equilibrium calculation. That is, for calculation shape of (P,T,X) that the scaling of (1,1,N) is similar to (1,sqrt(N),sqrt(N)). Each point is sampled twice and averaged.

It appears that multiprocessing would be a poor choice for default.

What are your thoughts then on client vs sync, where with client we will have to implicitly start a distributed client. I think maybe sync as the default is okay given that the biggest speedup was less than 2x even for 10k equilibrium calculations on 8 cores.

al-ni
cu-zr_sau
la-ti
mg-zn

@bocklund

This comment has been minimized.

Collaborator

bocklund commented Jul 25, 2017

Log-log because why not
al-ni-log
cu-zr_sau-log
la-ti-log
mg-zn-log

@richardotis

This comment has been minimized.

Collaborator

richardotis commented Jul 28, 2017

@bocklund Thanks for running this test. It appears that "sync" would be a sensible default up to, perhaps, 100 calculations, at which point we probably should switch to the distributed scheduler.

What do you think about leaving our current default scheduler (distributed), and just increasing the threshold num_calcs value for switching to the scheduler?

@richardotis

This comment has been minimized.

Collaborator

richardotis commented Jul 28, 2017

On a second read, it would appear that that value only controls the way calculations are chunked, so perhaps changing the default scheduler to "sync" is the best option.

@bocklund

This comment has been minimized.

Collaborator

bocklund commented Jul 28, 2017

In OOB discussion:

  • Possible poor chunking here
  • Certain conditions_per_chunk_per_axis cause an error to be raised.
@richardotis

This comment has been minimized.

Collaborator

richardotis commented Jul 31, 2017

I double checked the chunking code and it is fine. (chunk_grid is actually an index array, not the actual chunk list.) Still not sure on conditions_per_chunk_per_axis.

@bocklund

This comment has been minimized.

Collaborator

bocklund commented Apr 3, 2018

Tried to look at this some more. It seems like we are chunking the way we intend by the graph (10 calculations split into 5 computations with 2 conditions each)

mydask

However I was running into some issues that seemed to be related to trying to pickle things when running on multiple processes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment