Allow user to control behavior when login fails; warn by default

[mfisher87]

This is very rough and rushed, would love feedback!

Also, better distinguish in the code between a failed login attempt and an unavailable strategy.

Resolves #888

Pull Request (PR) draft checklist - click to expand

• Please review our
  contributing documentation
  before getting started.
• Populate a descriptive title. For example, instead of "Updated README.md", use a
  title such as "Add testing details to the contributor section of the README".
  Example PRs: #763
• Populate the body of the pull request with:
  • A clear description of the change you are proposing.
  • Links to any issues resolved by this PR with text in the PR description, for
    example closes #1. See
    GitHub docs - Linking a pull request to an issue.
• Update CHANGELOG.md with details about your change in a section titled
  ## Unreleased. If such a section does not exist, please create one. Follow
  Common Changelog for your additions.
  Example PRs: #763
• Update the documentation and/or the README.md with details of changes to the
  earthaccess interface, if any. Consider new environment variables, function names,
  decorators, etc.

Click the "Ready for review" button at the bottom of the "Conversation" tab in GitHub
once these requirements are fulfilled. Don't worry if you see any test failures in
GitHub at this point!

Pull Request (PR) merge checklist - click to expand

Please do your best to complete these requirements! If you need help with any of these
requirements, you can ping the @nsidc/earthaccess-support team in a comment and we
will help you out!

• Add unit tests for any new features.
• Apply formatting and linting autofixes. You can add a GitHub comment in this Pull
  Request containing "pre-commit.ci autofix" to automate this.
• Ensure all automated PR checks (seen at the bottom of the "conversation" tab) pass.
• Get at least one approving review.

---

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

[github-actions]

👈 Launch a binder notebook on this branch for commit 58efc88

I will automatically update this comment whenever this PR is modified

👈 Launch a binder notebook on this branch for commit 2571e10

👈 Launch a binder notebook on this branch for commit 775976e

👈 Launch a binder notebook on this branch for commit a8b204c

👈 Launch a binder notebook on this branch for commit f8e74b4

👈 Launch a binder notebook on this branch for commit 6f2cabf

👈 Launch a binder notebook on this branch for commit 763fcbe

👈 Launch a binder notebook on this branch for commit ce1b44c

👈 Launch a binder notebook on this branch for commit cca538f

👈 Launch a binder notebook on this branch for commit 720241d

👈 Launch a binder notebook on this branch for commit f234231

👈 Launch a binder notebook on this branch for commit bc18781

👈 Launch a binder notebook on this branch for commit 3154e47

👈 Launch a binder notebook on this branch for commit 2cf8fc6

👈 Launch a binder notebook on this branch for commit 315424c

👈 Launch a binder notebook on this branch for commit 5539a25

👈 Launch a binder notebook on this branch for commit 57535b2

👈 Launch a binder notebook on this branch for commit 1c5de64

👈 Launch a binder notebook on this branch for commit 6ce736a

👈 Launch a binder notebook on this branch for commit 3c69ed2

[mfisher87]

We don't want to skip without regard for what type of error occurs; instead I'm using the exception type to communicate from the inner call whether we skip or not. There's a difference between "strategy unavailable", "something unexpected happened with strategy", and "login failed", and we want the user and programmer to be able to tell!

[mfisher87]

Any ideas why 3.13 integration tests are failing? I'm confused :)

[chuckwondo]

Thanks for picking up this issue.

Given that not raising an error is what creates confusion and issue/discussion posts, I suggest we make "error" the default instead.

I suspect this will not only eliminate the bulk (if not all) of the confusion around the current behavior, but will be more in line with what is common behavior in other libraries that preemptively attempt to auth.

I would also suggest simply using a boolean (perhaps, raise_on_failure: bool = True). I think it would be a real stretch for there to be more than 2 ways to handle a failed login (i.e., either raise or don't).

I realize that making raising an error the default behavior qualifies as a breaking change, but in this case, I believe it would be the more familiar and less problematic default.

In other words, if we add this functionality with "warn" as the default, we're not breaking anything, but I would argue that we're perhaps not likely to reduce the number of questions about why things aren't working when login appears to have succeeded.

Further, either way, some folks are likely to need/want to change code. If we make "warn" the default, folks who want to end their confusion/frustration must change their code to set the option to "error" (and also continue to do so in future code). If we make "error" the default, the same users don't have to change code. Rather, they simply upgrade earthaccess.

Perhaps this is worthy of opening a discussion (unless I missed it).

[chuckwondo]

Any ideas why 3.13 integration tests are failing? I'm confused :)

I see this, but don't know why this would not be the case for the other runs:

aiohttp.client_exceptions.ClientResponseError: 401, message='Unauthorized', url='https://n5eil01u.ecs.nsidc.org/DP4/MOST/MOD10A1.061/2000.02.24/MOD10A1.A2000055.h34v07.061.2020037173428.hdf'

However, there may be another problem (likely unrelated) with the int. tests. As far as I can tell, all of the int. tests are running Python 3.11, not their specific version, and I suspect that's because the "Install uv" step ends up using the same cache across all versioned runs of the int. tests. We likely need to use a cache suffix that at a minimum includes the matrix python version so that each version has its own cache. See https://github.com/astral-sh/setup-uv?tab=readme-ov-file#enable-caching

[mfisher87]

Agreed with everything in your first post. Thank you for sharing your reasoning!

[mfisher87]

#947 for your second comment @chuckwondo

addressed!

[mfisher87]

pre-commit.ci autofix

[mfisher87]

pre-commit.ci autofix

[jhkennedy]

Looks good to me! @chuckwondo did you want to weigh in again?

[mfisher87]

@chuckwondo if you're good, go ahead and merge. Otherwise I'll merge it tomorrow before releasing!

[chuckwondo]

@chuckwondo if you're good, go ahead and merge. Otherwise I'll merge it tomorrow before releasing!

Looks good, but of course I have to throw another (minor) wrench into the works :-)

I don't feel terribly strongly about the following, so I can likely be convinced to forget about it, but given that we are now raising an error by default, I'd actually suggest we not add the raise_on_failure option.

My reasoning is that the language already provides a familiar way to handle this: try/except.

I'm taking a wild guess here, but I suspect nobody (or perhaps a nearly-zero percentage of users) check the result of the call to login to see if the Auth instance returned is authenticated (currently the only way to tell if login failed).

For example, I suspect (nearly) nobody is doing this, but please correct me if you've seen otherwise:

import earthaccess

auth = earthaccess.login()

if not auth.authenticated:
    raise SomeException()

# do stuff requiring successful login
# ...

Instead, I suspect this is the common coding pattern, given the number of people who report having the problem that a failed login does "nothing":

import earthaccess

earthaccess.login()

# do stuff requiring successful login
# ...

If the former pattern were common, (almost) nobody would be "complaining" about this issue.

If my suspicion is current, then (almost) nobody would then bother to make the following code change, once this PR (as it stands) were to land:

import earthaccess

earthaccess.login(raise_on_error=False)

# do stuff requiring successful login
# ...

Conversely, if somebody does indeed use the latter approach (i.e., check the value of auth.authenticated), then they can simply remove the check and simply let the error propagate. Alternatively, if such a user is checking the value of auth.authenticated and taking a different course of action, then they would simply replace the check with a try/except block.

Bottom line: I don't believe there's a solid reason for us to bother adding a raise_on_failure option. Simply always raise on failure and allow the user to deal with it as they see fit via try/except (or not).

[mfisher87]

That's an excellent point. We don't need to clutter our API for this. Thanks for the 🧠 time Chuck ❤️