Skip to content

Show progress bar in interactive sessions (Jupyter, REPL) by default - #1065

Merged
betolink merged 15 commits into
earthaccess-dev:mainfrom
betolink:progress-bar
Aug 13, 2025
Merged

Show progress bar in interactive sessions (Jupyter, REPL) by default#1065
betolink merged 15 commits into
earthaccess-dev:mainfrom
betolink:progress-bar

Conversation

@betolink

@betolink betolink commented Aug 8, 2025

Copy link
Copy Markdown
Member

This is a minor patch to restore the progress bar when we open and download files, the default is to show it if we are in a notebook and there is a new parameter to mute it with hide_progress in both download() and open().


📚 Documentation preview 📚: https://earthaccess--1065.org.readthedocs.build/en/1065/

@github-actions

github-actions Bot commented Aug 8, 2025

Copy link
Copy Markdown

Binder 👈 Launch a binder notebook on this branch for commit 9888fc1

@betolink
betolink marked this pull request as ready for review August 8, 2025 19:48
@betolink

betolink commented Aug 8, 2025

Copy link
Copy Markdown
Member Author

We need to merge this before #1061 also the integration tests are failing due NSIDC ECS, they got hidden recently in favor of cloud copies but... that's coming in #1061 so I don't mind merging this with them failing.

@betolink betolink mentioned this pull request Aug 8, 2025
9 tasks
@mfisher87 mfisher87 changed the title Progress bar defaults to True when the session is interactive (Jupyter, RELP etc) Show progress bar in interactive sessions (Jupyter, REPL etc) by default Aug 9, 2025
@mfisher87 mfisher87 changed the title Show progress bar in interactive sessions (Jupyter, REPL etc) by default Show progress bar in interactive sessions (Jupyter, REPL) by default Aug 9, 2025
Comment thread earthaccess/api.py Outdated
provider: Optional[str] = None,
threads: int = 8,
*,
hide_progress: bool = False,

@mfisher87 mfisher87 Aug 9, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't feel we should add a new kwarg here. Currently pqdm_kwargs.disable allows the user to control this behavior, and perhaps this PR should just update the default value instead of introducing a new API element?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I.e.

pqdm_kwargs = {
    "n_jobs": threads,
    "disable": not _is_interactive(),
    **(pqdm_kwargs or {}),
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

makes sense, I feel like pqdm is kind of "obscure" and hide_progress is very explicit.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's definitely true. You've kinda convinced me :) this is end-user-facing and we want it to be user-friendly. Maybe it should be show_progress instead? hide_progress=False is a double-negative.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@mfisher87 it will be great if we can merge this to unblock #1061 and try to release v0.15.0 at the end of the week!

@mfisher87 mfisher87 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I may not be able to do another pass this week. I have events going on every day after today 😅

Comment thread CHANGELOG.md Outdated
Comment thread earthaccess/api.py Outdated
Comment thread earthaccess/store.py Outdated
except ImportError:
pass

import sys

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Move import to the top of the module?

Comment thread earthaccess/store.py
**(pqdm_kwargs or {}),
}

return pqdm(url_mapping.items(), multi_thread_open, **pqdm_kwargs)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

pqdm_kwargs could be None at this point, so it's possible the user will be NoneType is not a mapping on this line.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think that's why the or {} is there no?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@betolink, correct, that's what the or {} is for.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@betolink I show that line as deleted, so can't it still be None?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

(paraphrasing an LLM) you're totally right! I think we should include the or {} in this line as well, technically this is not a public method and the methods that call it have the or {}, should we open an issue for it?

Comment thread earthaccess/store.py Outdated
Comment thread earthaccess/store.py Outdated
Comment thread earthaccess/store.py Outdated
betolink and others added 6 commits August 12, 2025 11:27
Co-authored-by: Matt Fisher <3608264+mfisher87@users.noreply.github.com>
Co-authored-by: Matt Fisher <3608264+mfisher87@users.noreply.github.com>
Co-authored-by: Matt Fisher <3608264+mfisher87@users.noreply.github.com>
Co-authored-by: Matt Fisher <3608264+mfisher87@users.noreply.github.com>
@betolink
betolink requested review from chuckwondo and removed request for Sherwin-14 August 12, 2025 16:48
@betolink

Copy link
Copy Markdown
Member Author

Hi @chuckwondo if you have time, it would be great to have your input on this one. Just a minor edit on how we expose the progress bar. I'm waiting on this to keep working on #1061

@chuckwondo

Copy link
Copy Markdown
Contributor

Thanks @betolink! I'll take a look at this today.

chuckwondo
chuckwondo previously approved these changes Aug 13, 2025

@chuckwondo chuckwondo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! Approving, with just a few minor suggestions.

Comment thread earthaccess/store.py Outdated
Comment thread earthaccess/store.py
**(pqdm_kwargs or {}),
}

return pqdm(url_mapping.items(), multi_thread_open, **pqdm_kwargs)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@betolink, correct, that's what the or {} is for.

Comment thread earthaccess/api.py Outdated
of a UUID4 value.
provider: if we download a list of URLs, we need to specify the provider.
threads: parallel number of threads to use to download the files, adjust as necessary, default = 8
show_progress: whether to display a progress bar. Defaults to `True` for interactive sessions, otherwise `False`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
show_progress: whether to display a progress bar. Defaults to `True` for interactive sessions, otherwise `False`.
show_progress: whether or not to display a progress bar. If not specified, defaults to `True` for interactive sessions (i.e., in a notebook or a python REPL session), otherwise `False`.

Please also duplicate this description to the rest of the places where show_progress was added so that the description is consistent everywhere. (There's a fair bit of variation/inconsistency across the other spots.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done! I'm taking a look at the pre-commit error.

@chuckwondo

Copy link
Copy Markdown
Contributor

@betolink, be sure to merge main into your branch, and address the pre-commit error.

Co-authored-by: Chuck Daniels <cjdaniels4@gmail.com>
@betolink

Copy link
Copy Markdown
Member Author

so weird, locally pre-commit passes
image

@betolink

Copy link
Copy Markdown
Member Author

Ah my origin/main is not in sync with earthaccess/main, fixing that now!

@betolink
betolink requested a review from chuckwondo August 13, 2025 16:24

@chuckwondo chuckwondo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @betolink! Looks good to me!

@betolink
betolink merged commit 7fa9c5a into earthaccess-dev:main Aug 13, 2025
8 of 11 checks passed
@github-project-automation github-project-automation Bot moved this to ✅ Done in earthaccess Mar 3, 2026
@mfisher87 mfisher87 removed this from earthaccess Mar 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants