Speed up creation of Nodes in the AiiDA ORM by ltalirz · Pull Request #2214 · aiidateam/aiida-core

ltalirz · 2018-11-17T21:50:07Z

move AiiDAManager from aiida.work to aiida.manage
introduce caching of .objects in aiida.orm.entities.Collection
cache default user in aiida.orm.user.User.Collection (fix Add caching of object collections #2216)

Benchmark on MacBook Pro, python3

before: 58.5s to create 10k nodes (57.2 spent inside Node.__init__)
after: 2.8s to create 10k nodes (1.7s spent inside Node.__init__)

The timing of 58.5s is significantly worse than what we wrote down during the tests with @szoupanos and @giovannipizzi (which was ~10s for 10k nodes), i.e. probably someone broke the caching.

Anyhow, the target we set then was: 2s for 10k nodes, which we have reached now on python3 (it's the 1.7s that should scale linearly with the number of nodes).

giovannipizzi

@ltalirz great!!
Can I suggest to add a step in travis (not necessarily a test, maybe an additional test) that runs the speed test (e.g. for creating 10k nodes, and for running say 10x a simple verdi command that a) require load_dbenv and b) does not require it, and output some timings? This might be great in the long term to go back in history and monitor when something broke speedup. Moreover, in Jenkins one can output "artifacts" that are stored, maybe also in travis there is something similar. If you don't have the time to do it but you're ok with this, feel free to copy this text in an issue (and even assign me).

giovannipizzi · 2018-11-18T09:39:02Z

I realize tests are falling, there is at least one relative import that has not been moved

ltalirz · 2018-11-18T13:48:10Z

For performance tests, I opened #2215

This PR is still a little bit dirty and before I move forward I have a few questions, perhaps also for @sphuber or @muhrin :

What exactly should be the scope of the AiiDAManager? Before this PR it looked backend-independent on the surface (I might be wrong) but now we are definitely adding backend-dependent stuff.
@muhrin's work was making it possible to specify the backend for getting objects. I guess we should retain this functionality in the AiiDA manager. To me this means we either
a. cache the backend in the AiiDA manager and use that and/or
b. make it possible to specify the backend when accessing the cache (and effectively have a per-backend cache)
If lots of stuff is going to end up here, Ideally, we should try to reuse some of the syntax used to access the uncached objects.
My purpose here was just to speed up Node() but the uncached User.objects.get_default() is used in many other places in the code. We'll need to decide on a policy of where to use cached objects and where not.

coveralls · 2018-11-18T14:06:55Z

Coverage increased (+0.02%) to 69.014% when pulling 2ff392a on ltalirz:speedup-node into def85b5 on aiidateam:provenance_redesign.

muhrin · 2018-11-18T14:20:24Z

Dear Leo, thanks for the work - looks good.

My response:

AiiDAManager is specifically act as the single place to construct/cache/access backend specific calsses. Particularly some which are quite complex to construct e.g. Runners and Communicators. So I'm not sure what exactly you mean by 'backend dependent' but if you mean it does different things based on the current backend (or more specifically profile) then that's exactly what it's there for.
This is a good point and needs a little discussion. For now AiiDAManager is a singleton of sorts (given that it only has class methods) that uses the current profile to make decisions but that precludes the ability to use it with different profiles simultanously...that said, this is kind of the point in that it's there to do the right thing based on the currently set profile (which could potentially be changed at runtime).
I would make this lazily cached in the User collection so it's fast for everyone using it

ltalirz · 2018-11-18T14:33:41Z

@muhrin thanks a lot for your thoughts!

Re 2.: Ok. So, the idea is that when you load a new profile (when this becomes possible...), the AiiDAManager cache will be reset, correct? I will add this to its docstring.

Re 3.: Could you elaborate on this a bit? In 1. you wrote that the AiiDAManager should be the single place to construct/cache/access backend specific classes.
So withlazily cached do you mean:

The default user will be cached inside the AiiDAManager
User.objects.get_default() will use/populate the cached default user in AiiDAManager._DEFAULT_USER

Furthermore, is there currently anything that invalidates the cache?
What if you pass to User.objects.get_default() a backend that is not the current one?

muhrin · 2018-11-18T14:39:03Z

Correct.
The dependency relationship was envisaged to be that AiiDAManager know about the backend but not the other way around (where possible). This would mean that AiiDAManager should eventually contain the construct_backend() method and I would imagine that it would be User.objects.get_default() that would contain the caching itself. If you do User.objects(my_backend).get_default() the first part (User.objects(my_backend)) actually passes back a separate collection instance which would itself contain the same caching logic as the default one. Currently calling with a custom backend will not result in the collection being cached after instantiation which would have serious performance implications so it's a target for future optimisation.

ltalirz · 2018-11-18T15:01:52Z

Currently calling with a custom backend will not result in the collection being cached after instantiation which would have serious performance implications so it's a target for future optimisation.

This holds even when calling with the default backend, correct? I.e. caching stuff inside User.objects currently does not work.

In [1]: from aiida.orm import User

In [2]: User.objects._test = 1

In [3]: User.objects._test
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-3-998f4695a253> in <module>()
----> 1 User.objects._test

AttributeError: 'Collection' object has no attribute '_test'

So, for the moment, should we cache the default user inside the AiiDAManager and use this cache from User.objects.get_default() with a notice to move this cache into User.objects in the future (and knowing that this temporary solution will not do the right thing when passing a non-default backend to User.objects)?

muhrin · 2018-11-19T11:48:07Z

Hmm...I thought it was there already but you're right, it's not. The place for caching (the default) objects collection is here:

https://github.com/aiidateam/aiida_core/blob/aa7907260906f01d8d7bb04b1b1421b24ab00d3f/aiida/orm/entities.py#L116

It's quick to add, happy to do it because the performance implications could be great.

If you get this PR through I can add the caching, you can track it here: #2216

I would avoid putting the caching in the AiiDAManager because 1) it will move anyway, and 2) I haven't really reflected on if this is even a good place to put things like this (although on the surface I don't see why not).

Now if a new collection is created via .objects it is cached for the next time in a lazy store. Similarly the default user for a User collection is cached after being created for the first time. This may lead to issues if the default user can change for a given profile during runtime, however for now we don't support this.

muhrin · 2018-11-29T14:35:51Z

Ok, ready to go. @ltalirz please have a look. @sphuber please review

ltalirz · 2018-11-29T17:04:15Z

Do we really need a wildcard here?
Perhaps just import the functions we want (I guess the constants defined in profile.py are not needed here (?))

Changed this to what we do in other prats of the code (which I borrowed from the asyncio codebase):

from .profile import * __all__ = profile.__all__

This way we delegate what gets imported to profile itself.

ltalirz · 2018-11-29T17:06:32Z

Do we need to introduce a different adjective (configured user) here?
Why not default user?

Edit: I see you just moved the function. Perhaps still a good time to rename it now?

Changed to default_user_email

ltalirz · 2018-11-29T17:07:45Z

Was this an exception before as well?
Should it be forbidden not to have a default user?

Edit: I see, it was...

Yes, it's forbidden. Can't store any nodes without one.

ltalirz · 2018-11-29T17:18:43Z

Is this stuff no longer necessary, this creation of a new_user object?

It's now done at the ORM level in the aiida/backends/testimplbase.py insert_data so it's backend independent...this is the way, ideally, all these things would be but we can't delete everything from ORM function calls.

ltalirz · 2018-11-29T17:20:03Z

@muhrin added a few comments, otherwise looks good to me (+ tested that node creation is fast again :-) )!
just the failing tests to fix...

sphuber

Minor comments as well

sphuber · 2018-11-29T14:57:02Z

maybe get rid of the print statements or make them debug/info logs

sphuber · 2018-11-29T14:57:52Z

left over debug print statements

sphuber · 2018-11-29T15:00:59Z

Is this comment still relevant if the accompanying line is commented out?

sphuber · 2018-11-29T18:02:05Z

Should we uncomment this assert, or if not just remove it

Uncommented

sphuber · 2018-11-29T18:03:23Z

wrong indentation in docstring

sphuber · 2018-11-29T18:03:53Z

delete commented import

sphuber · 2018-11-29T18:07:16Z

This can be done more succinctly now

computer = Computer.get('deneb') transport = computer.get_transport() scheduler = computer.get_scheduler() scheduler.set_transport(transport)

This way we have greater control over its creation and we don't have to implement a reset method which starts to get complicate. We just throw the object away and instsantiate a new one when we need to reset * Removed construct_backend() (replaced by get_manager().get_backend()) * Changed all tests to not specify the backend, they should just do what user facing code does which is use the default one and if we ever want to test with different backends we just implement a function to load a new backend from a profile globally * Moved getting default user email to the `Profile` class

PR aiidateam#2214 stripped of safeguards that prevented the default user from being deleted in between tests. Since AiiDA expects a default user in many operations, leaving the DB in a state with no default user is not ideal. This commit reintroduces safeguards that result in the default user remaining untouched.

ltalirz requested review from giovannipizzi and sphuber November 17, 2018 21:50

giovannipizzi previously approved these changes Nov 18, 2018

View reviewed changes

ltalirz dismissed giovannipizzi’s stale review via 3f15745 November 18, 2018 13:10

ltalirz changed the title ~~Speed up creation of Nodes in the AiiDA ORM~~ [WIP] Speed up creation of Nodes in the AiiDA ORM Nov 18, 2018

ltalirz mentioned this pull request Nov 26, 2018

Add caching of object collections #2216

Closed

muhrin force-pushed the speedup-node branch from ade894d to 4c3eddb Compare November 29, 2018 14:34

muhrin changed the title ~~[WIP] Speed up creation of Nodes in the AiiDA ORM~~ Speed up creation of Nodes in the AiiDA ORM Nov 29, 2018

ltalirz commented Nov 29, 2018

View reviewed changes

sphuber requested changes Nov 29, 2018

View reviewed changes

muhrin force-pushed the speedup-node branch from 4c3eddb to 2ff392a Compare November 30, 2018 10:56

sphuber approved these changes Nov 30, 2018

View reviewed changes

sphuber merged commit 0cce69f into aiidateam:provenance_redesign Nov 30, 2018

sphuber mentioned this pull request Dec 3, 2018

Put construct_backend in AiiDAManager #2194

Closed

giovannipizzi mentioned this pull request Feb 13, 2019

running the tests deletes the default user #2407

Closed

ltalirz mentioned this pull request Feb 14, 2019

Prevent tests from deleting the default user #2480

Closed

Conversation

ltalirz commented Nov 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmark on MacBook Pro, python3

Uh oh!

giovannipizzi left a comment

Choose a reason for hiding this comment

Uh oh!

giovannipizzi commented Nov 18, 2018

Uh oh!

ltalirz commented Nov 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

muhrin commented Nov 18, 2018

Uh oh!

ltalirz commented Nov 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

muhrin commented Nov 18, 2018

Uh oh!

ltalirz commented Nov 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

muhrin commented Nov 19, 2018

Uh oh!

muhrin commented Nov 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ltalirz Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ltalirz Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ltalirz commented Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sphuber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muhrin Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muhrin Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ltalirz commented Nov 17, 2018 •

edited

Loading

ltalirz commented Nov 18, 2018 •

edited

Loading

coveralls commented Nov 18, 2018 •

edited

Loading

ltalirz commented Nov 18, 2018 •

edited

Loading

ltalirz commented Nov 18, 2018 •

edited

Loading

ltalirz Nov 29, 2018 •

edited

Loading

ltalirz Nov 29, 2018 •

edited

Loading

ltalirz commented Nov 29, 2018 •

edited

Loading

muhrin Nov 30, 2018 •

edited

Loading

muhrin Nov 30, 2018 •

edited

Loading