[Fix #469] Print log progress on import rake task #787

Vitalina-Vakulchyk · 2021-04-15T16:53:35Z

Allow printing of import progress during rake chewy:reset and rake chewy:parallel:reset.

The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Updated tests.
Added an entry to the changelog if the new code introduces user-observable changes. See changelog entry format for details.

barthez · 2021-04-16T12:45:38Z

lib/chewy/type/adapter/object.rb

+            args.flatten(1).compact
+          end
+
+          collection.count


Won't it be a "bit" inefficient for huge data sources?

Actually is hard to tell the advantages/disadvantages, because for huge data sources it could be particularly more interesting having a progress bar instead of just waiting without no feedback.

But yes, counting could take some time... But at least this is only calculated if someone asks for a progress bar.

WDYT? Keep as this or do you have other suggestion?

Maybe, somebody will feel that calculations take too much time and then will turn off the progressbar.

Or is it possible and does it makes sence to count the seconds of collection.count executing? And we can stop the process in case of too long duration (like 1 seconds) and pass (for example) 1 as resulted value. So progressbar.title will be set to Too many elements to output progressbar.

In this case we will see just this title without progressbar line in the console.

I have added code like this here:
https://github.com/toptal/chewy/blob/print-log-progress-on-import-rake-task/lib/chewy/type/import.rb#L159-L168

@barthez @dalthon

@barthez
Should I change the approach and just output the count of imported records to avoid collection.count? Or any other way?

And I can try to use Mutex to synchronize the count of records to output during parallel import.
@dalthon

I am totally ok with any of those approaches:

Using the progress bar not as default so coping with the possibility of this performance cost happens with some kind of consent

Changing this to a simple count of elements imported, of course handling the concurrency edge cases

Having progress bar or count depending on env parameters, having simple count as default

Since @barthez doesn't seem to much fan of this introduced extra query for count, I guess the most promising of those 3 points are the 2nd and 3rd.

Between those 2 last, I prefer the 2nd due to its simplicity and it would not add any new dependency to the project

Bartek suggested to use thread to increment progressbar.total eventually during parallel import executing:
progressbar = ProgressBar.create total: nil if routine.options[:progressbar] == 'true'
Thread.new { progressbar.total = adapter.import_count(objects) if progressbar }
117b75f

barthez · 2021-04-16T12:47:01Z

lib/chewy/type/import.rb

            adapter.import(*objects, routine.options) do |action_objects|
              routine.process(**action_objects)
+              progressbar.progress += action_objects.values.flatten.length if routine.options[:progressbar]


maybe? flatten is bad, maybe flatten(1) instead?

Suggested change

progressbar.progress += action_objects.values.flatten.length if routine.options[:progressbar]

progressbar.progress += action_objects.map { _, v| v.size }.sum if routine.options[:progressbar]

If you chose to apply it, do it in other places too.

Thank you! I applyed this change.

Is this approach better because it is more explicit? @barthez

dalthon

LGTM, just asked for a few tests, if it doesn't bother you much

dalthon · 2021-04-16T13:16:36Z

lib/chewy/type/adapter/object.rb

+            args.flatten(1).compact
+          end
+
+          collection.count


Actually is hard to tell the advantages/disadvantages, because for huge data sources it could be particularly more interesting having a progress bar instead of just waiting without no feedback.

But yes, counting could take some time... But at least this is only calculated if someone asks for a progress bar.

WDYT? Keep as this or do you have other suggestion?

dalthon · 2021-04-16T13:20:40Z

lib/chewy/type/import.rb

@@ -150,8 +151,10 @@ def empty_objects_or_scope?(objects_or_scope)

        def import_linear(objects, routine)
          ActiveSupport::Notifications.instrument 'import_objects.chewy', type: self do |payload|
+            progressbar = ProgressBar.create total: adapter.import_count(objects) if routine.options[:progressbar]


It would be nice to add a test case that mocks this progressbar object and make sure it has its progress incremented many times as expected

lib/chewy/rake_helper.rb

…nt while import

…ring import" This reverts commit 9e30ae1.

Vitalina-Vakulchyk · 2021-04-23T09:35:26Z

@barthez Bartek, please take a look again on the PR.

dalthon

LGTM!
Just added a few minor comments

Gemfile

gemfiles/rails.6.0.activerecord.gemfile

dalthon · 2021-04-23T09:37:11Z

spec/support/active_record.rb

@@ -1,6 +1,6 @@
 require 'database_cleaner'

-ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: 'file::memory:?cache=shared', pool: 10)
+ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: 'file::memory:?cache=shared', pool: 1)


It might be interesting to explain here that test suite doesn't run properly with more than one database connection, otherwise it does not find tables

rabotyaga

I think merge was a little bit hasty.
Mentioning the feature in the README would be nice.
Why did we switch import parallelization from a process-based to a thread-based one?

rabotyaga · 2021-04-23T12:17:04Z

CHANGELOG.md

@@ -33,6 +33,9 @@
        * in rake tasks output (e.g. `Imported CitiesIndex::City in 1s, stats: index 3` -> `Imported CitiesIndex in 1s, stats: index 3`)
    * Use index name instead of type name in loader additional scope
        * e.g. `CitiesIndex.filter(...).load(city: {scope: City.where(...)})` -> `CitiesIndex.filter(...).load(cities: {scope: City.where(...)})`
+  * [#469](https://github.com/toptal/chewy/issues/469): Add ability to output progressbar with `ENV['PROGRESS']` during `reset` rake tasks ([@Vitalina-Vakulchyk][]):
+    * for `rake chewy:reset` and `rake chewy:parallel:reset`
+    * progressbar is hidden by default, set `ENV['PROGRESS']` to `true` to display it


This should have been in the unreleased section, between lines 7 and 9. Or probably better in the new features sections (5-7).

Vitalina-Vakulchyk · 2021-04-23T12:46:56Z

@rabotyaga Because variables are protected from change for processes. I took the information from Readme of Parallel gem
https://github.com/grosser/parallel

Vitalina-Vakulchyk · 2021-04-23T12:49:28Z

@rabotyaga Thank you
I will create a PR to update CHANGELOG and Readme files.

…ion in parallel import

…ion in parallel import (#800)

…nce degradation in parallel import (toptal#800)

Vitalina-Vakulchyk added the WIP label Apr 15, 2021

Vitalina-Vakulchyk force-pushed the print-log-progress-on-import-rake-task branch from 55a36dc to 6455a0b Compare April 16, 2021 11:32

Vitalina-Vakulchyk removed the WIP label Apr 16, 2021

Vitalina-Vakulchyk force-pushed the print-log-progress-on-import-rake-task branch 2 times, most recently from 209859d to 2c00e1b Compare April 16, 2021 12:07

barthez reviewed Apr 16, 2021

View reviewed changes

dalthon reviewed Apr 16, 2021

View reviewed changes

Vitalina-Vakulchyk force-pushed the print-log-progress-on-import-rake-task branch 2 times, most recently from 3a7b5ab to d141602 Compare April 21, 2021 11:15

Vitalina-Vakulchyk added 2 commits April 21, 2021 18:31

[Fix #469] Print log progress on import rake task

34e8f5c

Resolve conflicts after rebasing

00f9f17

Vitalina-Vakulchyk force-pushed the print-log-progress-on-import-rake-task branch from 76670b0 to 057142f Compare April 22, 2021 08:18

Change type option to index for ActiveSupport::Notifications.instrume…

0b3a054

…nt while import

Vitalina-Vakulchyk force-pushed the print-log-progress-on-import-rake-task branch from 057142f to 0b3a054 Compare April 22, 2021 08:56

Vitalina-Vakulchyk and others added 15 commits April 22, 2021 12:37

Add thread_safe_progress_bar file

05ad877

Fix argument order of IMPORT_WORKER block

65b7956

Add title method to ThreadSafeProgressBar class and use it during import

9e30ae1

Revert "Add title method to ThreadSafeProgressBar class and use it du…

c346ece

…ring import" This reverts commit 9e30ae1.

Comment progressbar output spec for Import

be5dc7d

Add sleep to progressbar spec

9e541b4

Increase sleep to 3 in progressbar spec

ed997f3

Temporary remove thread for ThreadSafeProgressBar class

97d52ad

Remove sleep to 3 in progressbar spec

757f0f4

WIP

f74a140

Inspect connection reuse

cca173c

This should pass

0c34085

WIP

e396e92

WIP

96f9beb

Remove rescue during import

d76dd3f

Vitalina-Vakulchyk requested a review from barthez April 23, 2021 09:34

dalthon approved these changes Apr 23, 2021

View reviewed changes

Remove pry

8740d69

barthez approved these changes Apr 23, 2021

View reviewed changes

Vitalina-Vakulchyk merged commit cb7e4c4 into master Apr 23, 2021

Vitalina-Vakulchyk deleted the print-log-progress-on-import-rake-task branch April 23, 2021 10:02

rabotyaga reviewed Apr 23, 2021

View reviewed changes

rabotyaga pushed a commit that referenced this pull request May 24, 2021

Revert [#787](#787) progressbar feature to avoid performance degradat…

5ab5fd4

…ion in parallel import

rabotyaga mentioned this pull request May 24, 2021

Revert [#787](https://github.com/toptal/chewy/pull/787) progressbar feature #800

Merged

5 tasks

rabotyaga pushed a commit that referenced this pull request May 24, 2021

Revert [#787](#787) progressbar feature to avoid performance degradat…

6de5f21

…ion in parallel import

rabotyaga pushed a commit that referenced this pull request May 24, 2021

Revert [#787](#787) progressbar feature to avoid performance degradat…

6133dd4

…ion in parallel import

rabotyaga pushed a commit that referenced this pull request May 24, 2021

Revert [#787](#787) progressbar feature to avoid performance degradat…

6e77065

…ion in parallel import (#800)

cyucelen pushed a commit to cyucelen/chewy that referenced this pull request Jan 28, 2023

[Fix toptal#469] Print log progress on import rake task (toptal#787)

1ebf48d

cyucelen pushed a commit to cyucelen/chewy that referenced this pull request Jan 28, 2023

Revert [toptal#787](toptal#787) progressbar feature to avoid performa…

6ee6b9f

…nce degradation in parallel import (toptal#800)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #469] Print log progress on import rake task #787

[Fix #469] Print log progress on import rake task #787

Vitalina-Vakulchyk commented Apr 15, 2021 •

edited

Loading

barthez Apr 16, 2021

dalthon Apr 16, 2021

Vitalina-Vakulchyk Apr 16, 2021 •

edited

Loading

Vitalina-Vakulchyk Apr 19, 2021 •

edited

Loading

dalthon Apr 19, 2021

Vitalina-Vakulchyk Apr 19, 2021 •

edited

Loading

barthez Apr 16, 2021

Vitalina-Vakulchyk Apr 16, 2021

dalthon left a comment

dalthon Apr 16, 2021

dalthon Apr 16, 2021

Vitalina-Vakulchyk commented Apr 23, 2021

dalthon left a comment

dalthon Apr 23, 2021 •

edited

Loading

rabotyaga left a comment

rabotyaga Apr 23, 2021

Vitalina-Vakulchyk commented Apr 23, 2021 •

edited

Loading

Vitalina-Vakulchyk commented Apr 23, 2021

	progressbar.progress += action_objects.values.flatten.length if routine.options[:progressbar]
	progressbar.progress += action_objects.map { _, v\| v.size }.sum if routine.options[:progressbar]

[Fix #469] Print log progress on import rake task #787

[Fix #469] Print log progress on import rake task #787

Conversation

Vitalina-Vakulchyk commented Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vitalina-Vakulchyk Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Vitalina-Vakulchyk Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vitalina-Vakulchyk Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalthon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vitalina-Vakulchyk commented Apr 23, 2021

dalthon left a comment

Choose a reason for hiding this comment

dalthon Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

rabotyaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vitalina-Vakulchyk commented Apr 23, 2021 • edited Loading

Vitalina-Vakulchyk commented Apr 23, 2021

Vitalina-Vakulchyk commented Apr 15, 2021 •

edited

Loading

Vitalina-Vakulchyk Apr 16, 2021 •

edited

Loading

Vitalina-Vakulchyk Apr 19, 2021 •

edited

Loading

Vitalina-Vakulchyk Apr 19, 2021 •

edited

Loading

dalthon Apr 23, 2021 •

edited

Loading

Vitalina-Vakulchyk commented Apr 23, 2021 •

edited

Loading