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

Add dump & restore to holders & simulations. #712

Merged
merged 9 commits into from
Aug 29, 2018
Merged

Add dump & restore to holders & simulations. #712

merged 9 commits into from
Aug 29, 2018

Conversation

eraviart
Copy link
Member

@eraviart eraviart commented Aug 21, 2018

New features


EDIT (@fpagnoux)

Dump:

from openfisca_core.tools.simulation_dumper import  dump_simulation, 
dump_simulation(simulation, '/path/to/directory')

Restore:

from openfisca_core.tools.simulation_dumper import restore_simulation
simulation_2 = restore_simulation('/path/to/directory', tax_benefit_system)

@benjello benjello requested a review from fpagnoux August 21, 2018 15:53
@benjello
Copy link
Member

@fpagnoux : I took the liberty to ask you to review this PR.
@eraviart coded this during a session at IPP.
We need your input before we go forward and we add higher level methods to SurveyScenario.

@fpagnoux fpagnoux self-assigned this Aug 21, 2018
@fpagnoux
Copy link
Member

Thanks for the PR @eraviart!

A few general questions @benjello before looking more into details:

  • With this implem, after we restore the simulation, nothing is loaded in memory and everything is kept on disk. Is that on purpose? This could significantly slow down calculations after restoring.
  • Entities compositions and roles are not stored, is that an issue?

@benjello
Copy link
Member

@fpagnoux : I knew that your input would be valuable !

  • You first point is a very good one. For performance issue we may want that the variables are kept in RAM when read.
  • The second point is another good one, we definitively want to keep those if we want to compute more variables involving several entities after restoring. I completely forgot that when explaing the problem to @eraviart.

@fpagnoux
Copy link
Member

Ok, I will try adjust the implem to take these two points into account :)

@fpagnoux fpagnoux force-pushed the dump-restore branch 4 times, most recently from e1dbbb4 to d11fa9f Compare August 27, 2018 21:26
@fpagnoux
Copy link
Member

Tested on France calculating revenu_disponible for 40000 persons, dumping and restoring takes a reasonable time (<10s)

Mahdi, do you want to try the branch sees if it works for you, or should we go ahead and ask for a review?

@benjello
Copy link
Member

Thanks @fpagnoux !

Do you confirm that when restoring a simulation, the variables are kept back in RAM/disk according to the new memroy_config only when recalled and not restored at once ?

I think we should ask for a review so I can go on and propagate to downstream packages.
I could definitively come back and ask for a few changes if needed by a use case.


def test_dump_restore_different_simulations_different_variables():
data_storage_dir = tempfile.mkdtemp(prefix = "openfisca_")
from .test_countries import tax_benefit_system
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use absolute import?

Copy link
Member

Choose a reason for hiding this comment

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

There are many more relative imports in tests. Should we migrate them all to absolute imports
@sandcha @fpagnoux ?

Copy link
Member

Choose a reason for hiding this comment

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

We can't use absolute imports as the tests are not packaged in openfisca_core.

@fpagnoux
Copy link
Member

Do you confirm that when restoring a simulation, the variables are kept back in RAM/disk according to the new memroy_config only when recalled and not restored at once ?

Yes, restore_simulation takes the same arguments than the Simulation constructor, and the data are restored through the usual put_in_cache method so the memory config will be respected.

@benjello
Copy link
Member

Looks good to merge then @fpagnoux !

@fpagnoux fpagnoux force-pushed the dump-restore branch 2 times, most recently from b7c832a to 7a05d63 Compare August 29, 2018 15:15
@fpagnoux fpagnoux merged commit 74041c3 into master Aug 29, 2018
@fpagnoux fpagnoux deleted the dump-restore branch August 29, 2018 15:19
@bonjourmauko bonjourmauko added the kind:feat A feature request, a feature deprecation label Sep 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feat A feature request, a feature deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a way to dump to/restore fom disk the state of a simulation
5 participants