Loader with aggregation and visual loading state #555

davidlatwe · 2020-06-21T11:48:37Z

This PR supersede and will close #553.
The problem in #533 was, quoting from #553 (comment):

One thing to be noted, this PR lists out all subsets then fetching versions later, so if there is subset that has no versions at all, it will remain in view. Although this is a rare case in production but better to avoid that. The another approach I am currently trying will not have this issue.

And this is the PR what was promised in that comment.

What's changed

Subset, version fetching speed improved by involving MongoDB's aggregation.
The subset, version fetching is running in worker thread and is breakable.
Added loading spinner on subset tree view, provide visual feed while waiting results from database, although you may not even sees it in most cases. (aggregation is fast !)
Displaying "No data" in subset tree view if no versioned subset found.
Previously, toggling "Enable Grouping" check box will refresh the view and query the database again, now it will only reset the model. This was benefit from caching documents in worker fetching thread implementation.
Disabled QTreeView header's ResizeToContents resize mode to improve the performance of layout changing. (Inherited from Non blocking loader #553)

And here's the visual result. Note that I am not in the office at the moment, so I mocked the loading time in order to see the loading spinner in view.

And sorry for Pype guys @mkolar, @iLLiCiTiT , need another review from you and test if this PR will cause any trouble to merge #550.

Please let me know what you think !

Instead of querying documents again when toggling subsets, just reuse previous fetched result.

To improve the performance of tree view.

davidlatwe · 2020-06-21T17:37:41Z

Hmmm, just had a stab on merging #550, seems not an easy task.

hound · 2020-06-23T07:24:19Z

avalon/tools/loader/widgets.py

+ painter = QtGui.QPainter(self.viewport())
+ rect = event.rect()
+ rect = QtCore.QRectF(rect.topLeft(), rect.bottomRight())
+ qtext_opt = QtGui.QTextOption(QtCore.Qt.AlignHCenter |


line break after binary operator

Either before or after line break, Hound barks. 😠

We use line break before so operators are on new line (both won't work with hound if hound is not set ;) ).

I would personally in this case use:

qtext_opt = QtGui.QTextOption( QtCore.Qt.AlignHCenter | QtCore.Qt.AlignVCenter )

iLLiCiTiT · 2020-06-25T15:51:58Z

avalon/tools/loader/model.py

+ {"$group": {"_id": "$parent",
+ "_version_id": {"$last": "$_id"},
+ "name": {"$last": "$name"},
+ "data": {"$last": "$data"},


I would add "type" key here, but don't have explanation why :)

iLLiCiTiT · 2020-06-25T15:57:40Z

avalon/tools/loader/model.py

+ _subsets.append(subset)
+ _ids.append(subset["_id"])
+
+ _pipeline = [


Would be great if there is (e.g. in avalon.lib) function for this pipeline to be usable in multiple places.

def last_version_pipeline(subset_ids): return [ # Find all versions of those subsets {"$match": {"type": "version", "parent": {"$in": subset_ids}}}, ... ]

But it is only nice to have, because it is not used in other places yet.

Maybe it could be in the avalon.io module. 🤔

iLLiCiTiT · 2020-06-25T16:02:46Z

avalon/tools/loader/widgets.py

+ def __init__(self, parent=None):
+ super(SubsetTreeView, self).__init__(parent=parent)
+ loading_gif = os.path.dirname(style.__file__) + "/svg/spinner-200.svg"
+ spinner = QtSvg.QSvgRenderer(loading_gif)


My experience with svg in Qt is that it is slow in comparison to png or gif. Recommendation is to implement rotating QLabel with png, on the other side I guess that one svg won't affect Qt much.

Yep, there's only one, and it's animation playback will be stopped once the loading is completed. So yeah, I think we are safe :P

iLLiCiTiT · 2020-06-25T16:12:55Z

avalon/tools/loader/model.py

+ some of the first level field may not be presented.
+
+ """
+ def _fetch():


_fetch should be class method, there is not much reason to have it inside.

I guess it's there because it acts more like a local lambda written as a function.

Also, if it were put on the class it shouldn't be a classmethod right? Because it uses variables/functions of the instance of the class inside of it (not how it refers to self). So if put on the class it would be a regular method.

Also, if it were put on the class it shouldn't be a classmethod right?

I meant as object method def _fetch(self):, not as class method, my fault.

it acts more like a local lambda written as a function

I see advantage of lamda but there's no advantage of that here, all _fetch need is self. Method fetch_subset_and_version has 4 lines in fact. All what these lines do, is preparation and launching another method in thread, but because the definition of launched method is inside it's harder to read (for me) what's really happening inside.

This is "formatting" comment of my preferences. I have no ambition to change mindset because I know it's hard to change mine :)

Looks right to me. Nesting a function tells me that it is only useful to the parent function, which seems to be the case here. If you put it in the class or instance, I would expect it to be useful standalone and by others, and require its own tests. The smaller the interface the better. Also, if Python did support multiline lambdas, this would be a good place for it.

iLLiCiTiT · 2020-06-25T16:24:12Z

Logic seems to be ok for me. I'll have to modify it to be able to test it in our fork, but at this moment I don't see any possibly breaking part. Notes I have are nice to have changes, nothing will happen if they're not accepted.

iLLiCiTiT · 2020-06-25T16:41:25Z

avalon/tools/loader/model.py

+ for subset in _subsets]
+ self.doc_fetched.emit()
+
+ self._doc_payload[:] = []


I am curious why this is used self._doc_payload[:] = [] and not just self._doc_payload = []?

EDITED:
Because [:] creates copy of list but assigning new value to it is strange to me.

This is basically an in-place assignment. It would be the same as doing:

my_list = ["a", "b", "c"] my_list.clear()

And this replaces the list in place:

a = ["a", "b"] b = a # Show mutability a.append("c") print(b) # ["a", "b", "c"] # Assign new value to a does not change value of b, because it's assigning a new list a = ["aa", "bb"] print(b) # ["a", "b", "c"] # But when using in place assignment it would, because it's changing the same mutable list. a = ["a", "b"] b = a a[:] = ["aa", "bb"] print(b) # ["aa", "bb"]

Edit: added example above of the in-place assignment that is not just clearing it

I believe the reason to do explicit in-place assignments is due to the "mutable list" potentially being shared to another thread and otherwise that one working on an older copy of a list that isn't actually the same list as self._doc_payload. However, because it doesn't seem to be passed around as an object I don't think that's actually the case. @davidlatwe is it redundant for this case?

There should be some informational topics online on replacing lists in-place with regards to its mutability if you're looking for more info.

However, because it doesn't seem to be passed around as an object I don't think that's actually the case.

Yes, it's true that there's no need to use in-place assignment, just re-using the same object for no special reason. Will change it for good.

iLLiCiTiT · 2020-06-26T08:12:19Z

I can confirm it works with current multiselection implementation.

davidlatwe · 2020-06-29T18:02:55Z

I can confirm it works with current multiselection implementation.

Nice ! 🎉

BigRoy · 2020-09-21T08:45:58Z

Should we get this PR ready for merging? I imagine @davidlatwe you've been using this in production since.
And likely @iLLiCiTiT adopted it in the meantime too?

iLLiCiTiT · 2020-09-21T08:57:58Z

And likely @iLLiCiTiT adopted it in the meantime too?

Yes, we use it.

mottosso · 2020-09-24T12:32:54Z

Hope you're not waiting for me, please go ahead and merge if it's ready!

BigRoy · 2020-09-30T04:04:02Z

Tested this and worked flawlessly here. @davidlatwe feel free to merge - or I'll merge in a few days.

davidlatwe · 2020-09-30T06:56:48Z

Merging this !!

davidlatwe added 11 commits June 9, 2020 22:16

Fetch subset and version with aggregation

77a3559

WIP: Add subsets loading spinner on asset

703c6eb

Implemented subsets loading in thread

f5ee56d

Implemented subset view loading spinner

fc5e108

Reuse fetched documents on grouping

77a8676

Instead of querying documents again when toggling subsets, just reuse previous fetched result.

Change ResizeToContents resize mode to default column width

2d7e0af

To improve the performance of tree view.

Change spinner animation

8dfb66d

Remove asset loading gif spinner

5bac974

Remove duplicated code

ae85b42

Implemented empty state and stop repaint if not loading

4b89898

Simplify subset loading state conditions

6f530a6

davidlatwe requested review from tokejepsen, mottosso, BigRoy and mkolar June 21, 2020 11:48

Remove unused import

2de45f8

tokejepsen approved these changes Jun 21, 2020

View reviewed changes

davidlatwe added 2 commits June 23, 2020 15:20

Fix for Qt compact

4de3464

Fix hound

79957b4

hound bot reviewed Jun 23, 2020

View reviewed changes

iLLiCiTiT reviewed Jun 25, 2020

View reviewed changes

iLLiCiTiT mentioned this pull request Jun 26, 2020

Feature/loader with aggregation loading state ynput/avalon-core#158

Merged

BigRoy mentioned this pull request Sep 21, 2020

Performance optimizations using Mongo Aggregation #562

Open

BigRoy mentioned this pull request Sep 30, 2020

Non blocking loader #553

Closed

davidlatwe merged commit 4f6c897 into getavalon:master Sep 30, 2020

davidlatwe deleted the loader-with-aggregation branch September 30, 2020 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loader with aggregation and visual loading state #555

Loader with aggregation and visual loading state #555

davidlatwe commented Jun 21, 2020

davidlatwe commented Jun 21, 2020

hound bot Jun 23, 2020

davidlatwe Jun 23, 2020

iLLiCiTiT Jun 23, 2020

iLLiCiTiT Jun 25, 2020

davidlatwe Jun 29, 2020

iLLiCiTiT Jun 25, 2020

davidlatwe Jun 29, 2020

iLLiCiTiT Jun 25, 2020

davidlatwe Jun 29, 2020

iLLiCiTiT Jun 25, 2020

BigRoy Jun 26, 2020

iLLiCiTiT Jun 26, 2020

mottosso Jun 26, 2020

iLLiCiTiT commented Jun 25, 2020

iLLiCiTiT Jun 25, 2020 •

edited

Loading

BigRoy Jun 26, 2020 •

edited

Loading

davidlatwe Jun 29, 2020

iLLiCiTiT commented Jun 26, 2020

davidlatwe commented Jun 29, 2020

BigRoy commented Sep 21, 2020

iLLiCiTiT commented Sep 21, 2020

mottosso commented Sep 24, 2020

BigRoy commented Sep 30, 2020

davidlatwe commented Sep 30, 2020

Loader with aggregation and visual loading state #555

Loader with aggregation and visual loading state #555

Conversation

davidlatwe commented Jun 21, 2020

What's changed

davidlatwe commented Jun 21, 2020

hound bot Jun 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iLLiCiTiT commented Jun 25, 2020

iLLiCiTiT Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

BigRoy Jun 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iLLiCiTiT commented Jun 26, 2020

davidlatwe commented Jun 29, 2020

BigRoy commented Sep 21, 2020

iLLiCiTiT commented Sep 21, 2020

mottosso commented Sep 24, 2020

BigRoy commented Sep 30, 2020

davidlatwe commented Sep 30, 2020

iLLiCiTiT Jun 25, 2020 •

edited

Loading

BigRoy Jun 26, 2020 •

edited

Loading