[ENH] benchmarks: add tqdm progress bar #6623

Spinachboul · 2024-06-16T15:07:43Z

Reference Issues/PRs

Fixes Issue #6560

What does this implement/fix? Explain your changes.

Adds progress bar using tqdm library

What should a reviewer concentrate their feedback on?

Focus on testing the progress while calling the API

Did you add any tests for the change?

Yes, tested the progress bar functionality using sample model registry and model validation

yarnabrina · 2024-06-16T17:00:43Z

sktime/benchmarking/_lib_mini_kotsu/run.py

@@ -6,6 +6,7 @@
 from typing import List, Optional, Union

 import pandas as pd
+from tqdm import tqdm


Is tqdm part of our core dependencies, or implied by those? If not, this is going to fail.

I'm afraid tqdm is an independent package

I'm not doubting that it's a package of it's own, but it could be implied by other dependencies of sktime. For example, statsforecast (part of forecasting subset) will imply tqdm and so on.

As far as I know none of the core dependencies, e.g. pandas, numpy, scikit-learn etc. will imply tqdm, so this line will fail CI. What you should do is move this import to inside your functions/methods and use tqdm only if it present. There is no need to make progress bars mandatory, especially if someone uses it in non-interactive setup.

fkiraly

I think this PR makes more substantial changes than just adding a progress bar? For instance, it changes the logger files or the structure of the code.

Change requests:

leave the logic unchanged in this PR. If you want to change or improve, you are much welcome, but it should be a separate PR.
I would recommend a refactor: factor out the loop into a single private function, that is, a single execution of validation_spec and model_spec. This will play well with parallelization efforts later.
we need to make sure that use of tqdm is optional, and the code still runs if tqdm is not installed.

Spinachboul · 2024-06-19T06:26:54Z

@fkiraly
Sure then! I'll keep the base logic of the code unchanged , but just add the progress bars in the console
Could I import the dependencies inside the functions to avoid CI errors during checks!

…nto add_feature

fkiraly · 2024-06-19T15:31:39Z

Could I import the dependencies inside the functions to avoid CI errors during checks!

Yes, I would also make sure that everything runs without the tqdm dependency as well.

Spinachboul · 2024-06-19T17:29:01Z

@fkiraly & @benHeid
I hope this should be the final commit before merging!
Please tell if any changes need to be made to the code.

fkiraly

Thanks!

I think there should be verbose mode and non-verbose mode, I would switch that with a verbose arg that can be True or False (default). If False, tqdm is not imported, and no prints are done.

I think this is quite easy to achieve, since you can do an easy switch on the generator used in the two for loops, e.g., if verbose: model_registry_generator = tqdm(...) etc

There should also be some consistency with the logger use.

Spinachboul · 2024-06-21T08:53:28Z

@fkiraly & @benHeid
Would this be to your liking
I have included a verbose as one of the params which will control the verbosity of the progress bars
If verbose = False, ,then the funtion tqdm defined would let the program remain functional else would invoke progress bars while executing the for loops
Also I have made the program consistent by using logger.info() instead of print statements

fkiraly · 2024-06-21T15:38:38Z

Would this be to your liking

This works but imo it is unnecessarily obscure. How about

if verbose and tqdm_available:
    validation_gen = tqdm(validation_registry.all())
else:
    validation_gen = validation_registry.all()

yarnabrina · 2024-06-21T16:02:03Z

I'm unable to see any conditions in the changes tab on this Github PR currently regarding verbose mode. I'm also unable to see any check for presence of tqdm soft dependency being installed or not.

Is it just me? @fkiraly are you able to see?

benHeid

Thank you for the contribution. I have two requests that would improve the user experience.

benHeid · 2024-06-21T18:54:15Z

sktime/benchmarking/_lib_mini_kotsu/run.py

-    for validation_spec in validation_registry.all():
+    print("Printing Validations...")
+    print("\n")
+    for validation_spec in tqdm(validation_registry.all()):


I would propose to avoid the print statements, you can add a description of the loop into the tqdm call.

benHeid · 2024-06-21T18:56:01Z

sktime/benchmarking/_lib_mini_kotsu/run.py

-        for model_spec in model_registry.all():
+        print("Running models...")
+        print("\n")
+        for model_spec in tqdm(model_registry.all()):


I would propose to set leave=False to the second loop. This would remove the inner progress bar after the loop is finished. This would avoid a wall of loops and the user has not to scroll up to see the outer loop.

fkiraly · 2024-06-21T19:28:17Z

I'm unable to see any conditions in the changes tab on this Github PR currently regarding verbose mode. I'm also unable to see any check for presence of tqdm soft dependency being installed or not.

I think the changes are only in the screenshot, which does not correspond ot the state of the fork. @Spinachboul, can you please make sure you update the PR to the most recent state?

Spinachboul · 2024-06-22T07:03:33Z

@fkiraly | @benHeid | @yarnabrina | @achieveordie
I have updated the PR in the following ways

Added the verbosity to the progress bars as suggested by @fkiraly
Removed all the print statements and addional logger info(), while adding desciptions to the progress bars as suggested by @benHeid
Also removed the code obscurity by importing tqdm additionally as suggested by @fkiraly
Even updated the params in the main function!

fkiraly · 2024-06-22T22:32:24Z

sktime/benchmarking/_lib_mini_kotsu/run.py

@@ -1,4 +1,3 @@
-"""Interface for running a registry of models on a registry of validations."""


why did you remove this line?

fkiraly · 2024-06-22T22:34:00Z

sktime/benchmarking/_lib_mini_kotsu/run.py

@@ -58,16 +61,42 @@ def run(
        results_df = pd.DataFrame(columns=["validation_id", "model_id", "runtime_secs"])
        results_df["runtime_secs"] = results_df["runtime_secs"].astype(int)

+    tqdm_available = False
+    if verbose:
+        try:


try/except is not good style, if you know the condition you want to check.
In that case, simply check the condition, you can do this with _check_soft_dependencies as I have recommended, kindly search the code base if you want to see examples.

fkiraly · 2024-06-22T22:34:49Z

sktime/benchmarking/_lib_mini_kotsu/run.py

@@ -79,15 +108,11 @@ def run(
            ):
                logger.info(
                    f"Skipping validation - model: "
-                    f"{validation_spec.id} - {model_spec.id}"
-                    ", as found prior result in results."


why are you removing the comma?

fkiraly · 2024-06-22T22:34:59Z

sktime/benchmarking/_lib_mini_kotsu/run.py

                )
                continue

-            logger.info(


why are you removing this?

fkiraly

Great! Left some comments above.

Spinachboul · 2024-06-22T23:55:28Z

Sure!
I'll fix them soon
Thanks for the review

Spinachboul · 2024-06-24T19:04:10Z

@fkiraly
Made the requested changes!
Please review

fkiraly · 2024-06-25T01:01:51Z

sktime/benchmarking/_lib_mini_kotsu/run.py

@@ -58,16 +65,37 @@ def run(
        results_df = pd.DataFrame(columns=["validation_id", "model_id", "runtime_secs"])
        results_df["runtime_secs"] = results_df["runtime_secs"].astype(int)

+    tqdm_available, _ = _check_soft_dependencies("tqdm", severity="warning")


I think the warning should be raised only if verbose, with a clear error message. Otherwise, I would just leave it silent.

If you want to warn, you should use warn from sktime.utils.warnings which receives a reference from self, so users can turn it off via set_config.

@fkiraly

try: results_df = pd.read_csv(results_path) except FileNotFoundError: results_df = pd.DataFrame(columns=["validation_id", "model_id", "runtime_secs"]) results_df["runtime_secs"] = results_df["runtime_secs"].astype(int) tqdm_available, _ = _check_soft_dependencies("tqdm", severity="none") if verbose and not tqdm_available: warn("tqdm is not installed. Continuing without progress bars.", logger) if tqdm_available: from tqdm import tqdm

Something like this ⬆ ?

This has not been committed!

Add Progress bar

da3365b

Spinachboul requested review from achieveordie, benHeid, fkiraly and yarnabrina as code owners June 16, 2024 15:07

yarnabrina reviewed Jun 16, 2024

View reviewed changes

fkiraly requested changes Jun 16, 2024

View reviewed changes

fkiraly changed the title ~~Add Progress bar~~ [ENH] benchmarks: add tqdm progress bar Jun 16, 2024

fkiraly added enhancement Adding new functionality module:metrics&benchmarking metrics and benchmarking modules labels Jun 16, 2024

Spinachboul and others added 4 commits June 19, 2024 18:03

Merge branch 'sktime:main' into add_feature

2fe06b1

added files

16464eb

Merge branch 'add_feature' of https://github.com/Spinachboul/sktime i…

22662f5

…nto add_feature

Add internal progress bars using tqdm

8f41c93

Spinachboul force-pushed the add_feature branch from 982a463 to 8f41c93 Compare June 19, 2024 13:35

fkiraly requested changes Jun 20, 2024

View reviewed changes

Merge branch 'sktime:main' into add_feature

dc855b7

benHeid requested changes Jun 21, 2024

View reviewed changes

Spinachboul and others added 2 commits June 22, 2024 12:16

Merge branch 'sktime:main' into add_feature

a0bd035

Update branch

7967bdb

fkiraly reviewed Jun 22, 2024

View reviewed changes

sktime/benchmarking/_lib_mini_kotsu/run.py Outdated

)

continue

logger.info(

Copy link

Collaborator

fkiraly Jun 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you removing this?

fkiraly requested changes Jun 22, 2024

View reviewed changes

Refactor

f025c64

fkiraly reviewed Jun 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] benchmarks: add tqdm progress bar #6623

[ENH] benchmarks: add tqdm progress bar #6623

Spinachboul commented Jun 16, 2024

yarnabrina Jun 16, 2024

Spinachboul Jun 16, 2024

yarnabrina Jun 16, 2024 •

edited

Loading

fkiraly left a comment •

edited

Loading

Spinachboul commented Jun 19, 2024

fkiraly commented Jun 19, 2024

Spinachboul commented Jun 19, 2024

fkiraly left a comment

Spinachboul commented Jun 21, 2024 •

edited

Loading

fkiraly commented Jun 21, 2024

yarnabrina commented Jun 21, 2024

benHeid left a comment

benHeid Jun 21, 2024

benHeid Jun 21, 2024

fkiraly commented Jun 21, 2024

Spinachboul commented Jun 22, 2024

fkiraly Jun 22, 2024

fkiraly Jun 22, 2024 •

edited

Loading

fkiraly Jun 22, 2024

fkiraly Jun 22, 2024

fkiraly left a comment

Spinachboul commented Jun 22, 2024

Spinachboul commented Jun 24, 2024

fkiraly Jun 25, 2024

fkiraly Jun 25, 2024

Spinachboul Jun 25, 2024

		@@ -1,4 +1,3 @@
		"""Interface for running a registry of models on a registry of validations."""

[ENH] benchmarks: add tqdm progress bar #6623

Are you sure you want to change the base?

[ENH] benchmarks: add tqdm progress bar #6623

Conversation

Spinachboul commented Jun 16, 2024

Reference Issues/PRs

What does this implement/fix? Explain your changes.

What should a reviewer concentrate their feedback on?

Did you add any tests for the change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yarnabrina Jun 16, 2024 • edited Loading

Choose a reason for hiding this comment

fkiraly left a comment • edited Loading

Choose a reason for hiding this comment

Spinachboul commented Jun 19, 2024

fkiraly commented Jun 19, 2024

Spinachboul commented Jun 19, 2024

fkiraly left a comment

Choose a reason for hiding this comment

Spinachboul commented Jun 21, 2024 • edited Loading

fkiraly commented Jun 21, 2024

yarnabrina commented Jun 21, 2024

benHeid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fkiraly commented Jun 21, 2024

Spinachboul commented Jun 22, 2024

Choose a reason for hiding this comment

fkiraly Jun 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fkiraly left a comment

Choose a reason for hiding this comment

Spinachboul commented Jun 22, 2024

Spinachboul commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yarnabrina Jun 16, 2024 •

edited

Loading

fkiraly left a comment •

edited

Loading

Spinachboul commented Jun 21, 2024 •

edited

Loading

fkiraly Jun 22, 2024 •

edited

Loading