Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quality of live: Improved error messages #1731

Merged
merged 2 commits into from
Nov 6, 2024

Conversation

romanc
Copy link
Contributor

@romanc romanc commented Nov 5, 2024

Small quality of live PR that improves two error messages. Especially if the state was called state (as per default), it was hard to parse at first sight

image

For some reason, yapf was also changing the formatting of other code parts. I thus made two commits

  1. Format the two files that I was about to edit.
  2. Improve the error messages in the freshly formatted files.

This allows to separate real changes from purely stylistic ones.

Would it be an idea to enforce linting with a GitHub Actions workflow? I'm happy to write an issue about this (and contribute early next year).

@romanc
Copy link
Contributor Author

romanc commented Nov 5, 2024

Not sure why GitHub Actions wasn't able to fetch the pace data. Works on my machine. Maybe just a network glitch. @tbennun would you mind re-running that workflow?

/cc @FlorianDeconinck FYI

@phschaad
Copy link
Collaborator

phschaad commented Nov 6, 2024

This has happened quite a few times the last 2 days. An occasional re-run fixes the issue, but there may be an underlying issue. I've restarted the workflow

@romanc
Copy link
Contributor Author

romanc commented Nov 6, 2024

This has happened quite a few times the last 2 days. An occasional re-run fixes the issue, but there may be an underlying issue. I've restarted the workflow

Hmm weird. Let me propose two things

1.wget would normally retry (as far as I know), but not with fatal errors (like the 503 that we've seen in this case). There's --retry-connrefused that we can leverage to even retry under these circumstances. I'll setup a quick PR for this since you say that this occurs often.
2. This data rarely changes. We could leverage GHA caches to avoid re-downloading the files and instead fetching them for a GHA cache. I'll have to discuss with @FlorianDeconinck time-budget wise. Shouldn't take too long, but is more than just adding a retry flag.

@phschaad
Copy link
Collaborator

phschaad commented Nov 6, 2024

This seems reasonable, good idea. Thanks! Using GHA caches could make this more robust

Copy link
Collaborator

@phschaad phschaad left a comment

Choose a reason for hiding this comment

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

These changes LGTM, thank you.

As for the point about running a formatter through GHA when PRs are merged: I will bring this point up in our weekly DaCe meeting to discuss, thank you for the suggestion.

@phschaad phschaad added this pull request to the merge queue Nov 6, 2024
@romanc
Copy link
Contributor Author

romanc commented Nov 6, 2024

As for the point about running a formatter through GHA when PRs are merged: I will bring this point up in our weekly DaCe meeting to discuss, thank you for the suggestion.

Thanks for bringing this to the weekly meeting.

Merged via the queue into spcl:main with commit 7c70423 Nov 6, 2024
10 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Nov 7, 2024
According to @phschaad, the pyFV3 workflow occasionally fails with
connection refused errors when attempting to download the test data set
for regression testing. This PR configures `wget` to retry even in case
of fatal connection errors (like 404 (not found) or 503 (connection
refused)).

Let's see if this helps in the short term. In the long run, we should
think about leveraging caches for that data because it rarely changes
and it's kind of a wast to download it every time from scratch.

Follow-up PR from
#1731 (comment).

/cc @FlorianDeconinck FYI

Co-authored-by: Roman Cattaneo <>
@romanc romanc deleted the romanc/qol-error-messages branch December 1, 2024 15:43
@romanc romanc mentioned this pull request Dec 1, 2024
github-merge-queue bot pushed a commit that referenced this pull request Dec 4, 2024
Found two unused imports and an unused variable. From other code, I
inferred that you follow the convention to prefix unused (return)
variables with underscores.

I noticed that running `yapf` (as suggested in the contribution
guidelines) modified the files in places that I didn't touch. I thus
separated the pure formatting changes in the first commit. As argued in
PR #1731, I think it would be
beneficial (for the project) to enforce formatting as part of the CI.
@phschaad not sure if you got to discuss this in the weekly DaCe
meeting. I started a discussion page
#1804 and I'm happy to
contribute a corresponding workflow early next year (assuming we agree).

---------

Co-authored-by: Roman Cattaneo <>
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.

2 participants