Skip to content

handle failed result post with error post#39

Merged
PaulJKathmann merged 9 commits intodevelopfrom
pk/error_handling
Jul 25, 2025
Merged

handle failed result post with error post#39
PaulJKathmann merged 9 commits intodevelopfrom
pk/error_handling

Conversation

@PaulJKathmann
Copy link
Copy Markdown
Contributor

@PaulJKathmann PaulJKathmann commented Jul 24, 2025

Before this PR

If posting the job result fails 5 times an exception gets raised but the forwarder doesn't know about it, since it never gets the message from the user code container.

After this PR

  • If the job posting fails 5 times (e.g. result too large) then the client tries 5 more times to post just a simple error message.

Possible downsides?

Are Docs needed?

@changelog-app
Copy link
Copy Markdown

changelog-app Bot commented Jul 24, 2025

Generate changelog in changelog/@unreleased

Type (Select exactly one)

  • Feature (Adding new functionality)
  • Improvement (Improving existing functionality)
  • Fix (Fixing an issue with existing functionality)
  • Break (Creating a new major version by breaking public APIs)
  • Deprecation (Removing functionality in a non-breaking way)
  • Migration (Automatically moving data/functionality to a new system)

Description

After this fix: If the job posting fails 5 times (e.g. result too large) then the client tries 5 more times to post just a simple error message.

Check the box to generate changelog(s)

  • Generate changelog entry

Comment thread compute_modules/client/internal_query_client.py Outdated
Comment thread compute_modules/client/internal_query_client.py Outdated
lauraAriasFdez
lauraAriasFdez previously approved these changes Jul 24, 2025
Copy link
Copy Markdown
Contributor

@lauraAriasFdez lauraAriasFdez left a comment

Choose a reason for hiding this comment

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

lgtm with minor nits! Lets test it I can show you how to build the sdk locally and download the whl file to test

@policy-bot policy-bot Bot dismissed lauraAriasFdez’s stale review July 25, 2025 14:17

Invalidated by push of 60b4a7b

@PaulJKathmann PaulJKathmann marked this pull request as ready for review July 25, 2025 18:53
self.logger.error(error)
except TypeError as e:
self.logger.error(f"Failed to serialize result to json: {str(e)}")
self.report_job_result(job_id, json.dumps(self.get_failed_query(e)).encode("utf-8"))
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.

we should move this to report_job_result_failed with a different error message of failed to deserialize so the failure to post is in the same call

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated accordingly and tested

Comment thread compute_modules/client/internal_query_client.py Outdated
Co-authored-by: lauraAriasFdez <52958827+lauraAriasFdez@users.noreply.github.com>
@PaulJKathmann PaulJKathmann merged commit e696dac into develop Jul 25, 2025
11 checks passed
@autorelease3
Copy link
Copy Markdown

autorelease3 Bot commented Jul 25, 2025

Released 0.26.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants