[GitHub] [superset] betodealmeida commented on a change in pull request #15719: feat: adding Progress Bar to Benchmark script

[GitBox <gi...@apache.org>]

betodealmeida commented on a change in pull request #15719:
URL: https://github.com/apache/superset/pull/15719#discussion_r670797410



##########
File path: scripts/benchmark_migration.py
##########
@@ -171,25 +172,29 @@ def main(
 
     min_entities = 10
     new_models: Dict[Type[Model], List[Model]] = defaultdict(list)
+    bar = ChargingBar("Processing", max=min_entities)
     while min_entities <= limit:
         downgrade(revision=down_revision)
         print(f"Running with at least {min_entities} entities of each model")
-        for model in models:
-            missing = min_entities - model_rows[model]
-            if missing > 0:
-                print(f"- Adding {missing} entities to the {model.__name__} model")
-                try:
-                    added_models = add_sample_rows(session, model, missing)
-                except Exception:
-                    session.rollback()
-                    raise
-                model_rows[model] = min_entities
-                session.commit()
-
-                if auto_cleanup:
-                    new_models[model].extend(added_models)
+        for i in range(min_entities):
+            for model in models:
+                missing = min_entities - model_rows[model]
+                if missing > 0:
+                    print(f"- Adding {missing} entities to the {model.__name__} model")
+                    try:
+                        added_models = add_sample_rows(session, model, missing)

Review comment:
       Oh, I don't think the progress bar idea is going to work. :(
   
   What we want to do is to increment it (call `bar.next()`) for each entity that is added. But that loop happens inside `add_sample_rows`.
   
   Something like this would work, but it's kind of ugly:
   
   ```python
   bar = ChargingBar("Processing", max=missing)
   try:
       added_models = add_sample_rows(session, model, missing, callback=bar.next)
   except Exception:
       session.rollback()
       raise
   bar.finish()
   ```
   
   And then we'd need to update `add_sample_rows` to:
   
   ```python
   def add_sample_rows(
       session: Session,
       model: Type[Model],
       count: int,
       callback: Callable[[None], None],
   ) -> List[Model]:
       """
       Add entities of a given model.
   
       :param Model model: a Superset/FAB model
       :param int count: how many entities to generate and insert
       """
       inspector = inspect(model)
   
       # select samples to copy relationship values
       relationships = inspector.relationships.items()
       samples = session.query(model).limit(count).all() if relationships else []
   
       entities: List[Model] = []
       max_primary_key: Optional[int] = None
       for i in range(count):
           callback()
   ```
   
   But having `callback` there seems a bit weird and is definitely not a common pattern.
   
   Having the progress bar inside `add_sample_rows` would lead to cleaner code, but we shouldn't mix IO with business logic — a function that adds entities should not have to take care of a progress bar.
   
   Sorry, I thought this would be more straightforward. :(




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org