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

Speed up startup and introduce progress bars #109

Merged
merged 3 commits into from Sep 9, 2014

Conversation

2 participants
@t-animal
Contributor

t-animal commented Sep 4, 2014

Ahoi!
Commit cea68db introduces progress bars while scanning for files and a spinner while initially discovering albums. This fixes issue #102 .
Commit d69bc98 shifts the parsing of the exif-data for sorting to later in the process, so that, if you don't want to sort, things are sped up dramatically (from ~3 minutes to some seconds on my 15k image library). Additionally this way the image count is known and a progress bar can be displayed.

@t-animal t-animal force-pushed the t-animal:master branch from d69bc98 to 6089bd2 Sep 4, 2014

@t-animal

This comment has been minimized.

Contributor

t-animal commented Sep 4, 2014

I've had some trouble testing due to an error, which was already present in tags/0.8.0 where sigal build would not run cleanly unless the plugin section in the config file was commented out. But I think this should not affect the commits anyway.

To reproduce:

    virtualenv venv
    git clone git@github.com:saimn/sigal.git
    . venv/bin/activate
    pip install sigal/
    cd sigal/tests/sample/
    sigal build

I've removed the offending lines, as they don't appear in the default config in templates/

I think these commits should not be squashed as they all do something different. And should be fast-forwardable. But if you strongly disagree, I will sqash them.

@saimn

This comment has been minimized.

Owner

saimn commented Sep 4, 2014

I've had some trouble testing due to an error, which was already present in tags/0.8.0 where sigal build would not run cleanly unless the plugin section in the config file was commented out.

oh, I completely missed that ... it happens only with the multiprocessing feature and if a plugin is used with an import, because then the settings dict can't be serialized (with pickle). But it can also happen if you define a class or import a module in the settings file.

@@ -281,6 +287,15 @@ def create_output_directories(self):
self.orig_path = join(self.dst_path, self.settings['orig_dir'])
check_or_create_dir(self.orig_path)
def sortMedias(self, medias_sort_attr):

This comment has been minimized.

@saimn

saimn Sep 4, 2014

Owner

-> sort_medias

@@ -459,7 +477,7 @@ def __init__(self, settings, ncpu=None):
if path not in albums.keys():
dirs.remove(d)
album = Album(relpath, settings, dirs, files, self)
album = Album(relpath, settings, dirs, files, self, False)

This comment has been minimized.

@saimn

saimn Sep 4, 2014

Owner

If I understand correctly, the sort feature is no more used ?

This comment has been minimized.

@t-animal

t-animal Sep 4, 2014

Contributor

Yes, it is. It's only been shifted from the constructor (at a time when the total amount of images is unknown) to a time when the total amount is known so we can display a progress bar. However I wanted to keep the interface the same, so I added this optional argument.

This comment has been minimized.

@saimn

saimn Sep 4, 2014

Owner

ok, but I think it's fine to remove it. The only difference is that the signal album_initialized will be called when the album is not sorted, but we could add a album_sorted at the end of sort_medias.

@property
def date(self):
if not self.exif:
self.raw_exif, self.exif = get_exif_tags(self.src_path)

This comment has been minimized.

@saimn

saimn Sep 4, 2014

Owner

This will break if the exif property is used in the theme and if the date property is not used (no sort by date).
self.exif and self.raw_exif also need to be properties that can be loaded lazily.

@t-animal t-animal force-pushed the t-animal:master branch from 5ef505f to 6089bd2 Sep 4, 2014

@@ -467,6 +485,10 @@ def __init__(self, settings, ncpu=None):
album.create_output_directories()
albums[relpath] = album
with progressbar(albums.values(), label="Sorting media") as progressAlbums:
for album in progressAlbums:
album.sortMedias(settings['medias_sort_attr'])

This comment has been minimized.

@saimn

saimn Sep 4, 2014

Owner

ah ok, the sort is done here. Why not removing the sort from the Album.__init__ ?

@t-animal t-animal force-pushed the t-animal:master branch from 6089bd2 to 73bede2 Sep 4, 2014

else:
self.logger.info('Album %r is empty', album)
with progressbar(self.albums.values(), label="Collecting files",
item_show_func=log_func) as albums:

This comment has been minimized.

@saimn

saimn Sep 4, 2014

Owner

With the item_show_func option which print the current album name, the output is quite ugly on a small terminal. What do you think about removing this ?

This comment has been minimized.

@t-animal

t-animal Sep 4, 2014

Contributor

Yes, I noticed this too. One possibility would be to truncate the length to click.get_terminal_size()[0]. I'll look into it.

@saimn

This comment has been minimized.

Owner

saimn commented Sep 4, 2014

@t-animal : thanks for the great job ! The main issue that remains is the verbose and debug modes (-d|-v) which are not usable in the current state. Do you know if there is a nice way to not use the progress bar in these modes ?

@t-animal

This comment has been minimized.

Contributor

t-animal commented Sep 4, 2014

Well I guess if you pass the file parameter something like /dev/null the progressbar should not be printed.

@t-animal t-animal force-pushed the t-animal:master branch from 73bede2 to fd0f0fb Sep 4, 2014

@t-animal

This comment has been minimized.

Contributor

t-animal commented Sep 5, 2014

Ok, so I've removed the sorting from the constructor entirely, truncated the name of the album to terminal width and hid the progress bar in verbose and debug mode.

@saimn

This comment has been minimized.

Owner

saimn commented Sep 7, 2014

Could you plz rebase on master to incorporate the latest change ? Currently the PR can't be merged, so there must be a conflict somewhere ;)
It works great (minus the issue with accentued filenames), the only (small) issue that I see is when a log.error/warning happen inside the loop, it breaks the progress bar. But I must sure if there is something to do, maybe just adding a space after the number of images (and before the log message).
Also I don't see the "Processing files" label, but this is maybe an issue with click.

@t-animal t-animal force-pushed the t-animal:master branch 5 times, most recently from af3ead7 to 67e453e Sep 8, 2014

@t-animal t-animal force-pushed the t-animal:master branch from 67e453e to 9b0098d Sep 8, 2014

@t-animal t-animal force-pushed the t-animal:master branch from 9b0098d to 9c2cb0e Sep 8, 2014

@t-animal

This comment has been minimized.

Contributor

t-animal commented Sep 8, 2014

Yes, this bugs me, too, because in my gallery there's a couple of files too big for PIL so it prints a DecompressionWarning each time, breaking the output. However this was already an issue before progress bars, it just didn't have such a big effect (but it was nevertheless annoying). Would be nice if this could be fixed... Maybe I'll look into it some time.

@saimn

This comment has been minimized.

Owner

saimn commented Sep 9, 2014

Thanks for the progress made on this feature.
I am not sure about the print "Collecting albums /-": I almost never see it (maybe because I have a SSD or you have a huge number of albums ;-)).
I also miss the previous printing with the number of images/videos per album, which was quite useful to have an idea of what will be processed. The previous print was also quite useful in verbose/debug mode.
Let's merge this and see how it is works with some practise. I wonder if it would not be better to keep only the "Processing files" progress bar.

saimn added a commit that referenced this pull request Sep 9, 2014

Merge pull request #109 from t-animal/master
Speed up startup and introduce progress bars

@saimn saimn merged commit 830315d into saimn:master Sep 9, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@saimn

This comment has been minimized.

Owner

saimn commented Sep 9, 2014

To give a preview, before:

-> Astro : 9 images
-> Climbing : 38 images, 1 videos
................................................
Done.
Processed 47 images (0 skipped) and 1 videos (0 skipped) in 63.73 seconds.

Now:

Sorting media  [####################################]  100%
Collecting files  [####################################]  100%          
Processing files  [####################################]  48/48          

Done.
Processed 47 images (0 skipped) and 1 videos (0 skipped) in 61.03 seconds.

Some feedback from users on what needs to be displayed would be greatly appreciated ! :)

@saimn saimn modified the milestone: 0.9 Aug 31, 2015

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