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

Multi-processes Exec Mode #105

Merged
merged 2 commits into from
Jun 15, 2020
Merged

Multi-processes Exec Mode #105

merged 2 commits into from
Jun 15, 2020

Conversation

fjribi
Copy link
Contributor

@fjribi fjribi commented Jun 1, 2020

PR concerning errors like ("NameError: name 'np' is not defined") when using multiprocess exec mode on windows:
Linked to #104 & #93

Is it convenient to add the following "workaround code" to [ENV]\Lib\site-packages\cadCAD_init_.py] file to fixe the issue?

name = "cadCAD"
configs = []

#added code
import os
if os.name == 'nt': 
    import dill
    dill.settings['recurse'] = True 

This fixed the issue.

or should we only add the code to

class ExecutionContext:
    def __init__(self, context: str = ExecutionMode.multi_proc) -> None:
...
        elif context == 'multi_proc':
           #add code here? 
           import os
           if os.name == 'nt': 
               import dill
               dill.settings['recurse'] = True 

            self.method = parallelize_simulations

to limit the setting to multiprocess exec mode and reduce potential side effects. Indeed, although this seems to be a stable workaround for windows, there are some comments (see [https://github.com/uqfoundation/dill/issues/115]) that are mitigated about the solution.

There are 2 other less convincing alternatives for our issue:
1 - To declare the missing global variables (which are not correctly serialized without dill.settings['recurse'] = True) at the function level (see [https://github.com/uqfoundation/multiprocess/issues/6]) => could be less practical, since this requires to re-declare global variables locally...
2 - to use cloudpickle instead of dill (see [https://github.com/apache/airflow/issues/7870]), => no idea on the impact of the migration

Regards,

Adding Contribution Guidelines & Nix
@JEJodesty JEJodesty merged commit e76c1e4 into staging Jun 15, 2020
@JEJodesty
Copy link
Member

JEJodesty commented Jun 17, 2020

@fjribi Accidentally merged this instead of converting to draft
Sorry for the confusion. I will link PR in the issue for context.

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.

2 participants