Add debugging message for #443 #448
Conversation
src/utils.js
Outdated
| @@ -78,6 +78,13 @@ const upload = async (opts) => { | |||
| const response = await fetch(endpoint, { method: 'POST', headers, body }); | |||
| const uri = await response.text(); | |||
|
|
|||
| if (!uri) { | |||
| console.log(response); | |||
There was a problem hiding this comment.
I would not log here and would throw the error message
There was a problem hiding this comment.
@DavidGOrtega, the issue is that there is no error message, but just an allegedly successful response with an empty body. That's why I chose to log the whole response object: it may contain precious information that might be hard to obtain on a (probably) intermittent bug like this.
Would you like to integrate it into the error message or just extract some properties?
There was a problem hiding this comment.
We should not log using console.log we should use a logging instead.
The error should bubble the issue to a better handler.
This is a technical debt that we have to solve with #22
- no console.log
- no log that can not be display by debug, info, error levels...
- prepare everything for json outputs choosing the right std (another log would break the output)
There was a problem hiding this comment.
the issue is that there is no error message, but just an allegedly successful response with an empty body. That's why I chose to log the whole response object: it may contain precious information that might be hard to obtain on a (probably) intermittent bug like this.
Let's advise that the request to the storage failed. I wonder if we could not also retry at least once 🤔
There was a problem hiding this comment.
We should not log using console.log we should use a logging instead.
Agreed! I used a plain log call because we haven't moved yet to winston as you proposed if I recall correctly.
The error should bubble the issue to a better handler.
Then, I guess that we should just throw an error with templated information and let the handler do the rest.
There was a problem hiding this comment.
I would not say it's not reliable, my guess is that its overloaded as any other SAAS.
With the current backend architecture, I struggle to find how a possible overload would produce an empty succesful reply. Anyhow, we can't discard that possibility.
I would put that defense in order to protect the workflow. Take into account that if the user is not setting any kind of model cache she would have to redo the whole training because the asset did not uploaded at the end of the workflow.
That's a really good point, though I don't know if this event is frequent enough to justify this kind of safeguard. I wouldn't be concerned at all by this if we could silently send the error through a telemetry system, but I suspect that #443 won't happen that often and it could be more beneficial in the long run to produce an error, ask users to report, and fix the issue whenever possible.
There was a problem hiding this comment.
I struggle to find how a possible overload would produce an empty succesful reply.
Do you have the code finally?
There was a problem hiding this comment.
Here. 😂 Sorry for the opaque reply, I thought that you noticed the thread in Slack.
This commit doesn't fix #443, but closes it until we can reproduce again the error.
0x2b3bfa0
force-pushed
the
debug-issue-443
branch
from
07f5484 to
c072ec4
Compare
…#431) * [create-pull-request] automated change (#425) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Update dependacron pull request step (#426) This commit updates the major version for the peter-evans/create-pull-request GitHub Action step. Required in order to avoid a fatal error caused by the hard deprecation of the `set-env` and `add-path` standard output commands after CVE-2020-15228. * Add sanity check for cml-publish with filesystem paths (#427) * Add sanity check for cml-publish files Closes #308 and probably also closes #401 * Restyled by prettier (#428) Co-authored-by: Restyled.io <[email protected]> * Add tests for cml-publish file errors * Also removes an stray leading dot on the introduced error message * Embrace native file error messages for cml-publish Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com> Co-authored-by: Restyled.io <[email protected]> * Add destroy timeout feature (#419) * [create-pull-request] automated change (#441) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Helio Machado <[email protected]> * Add support for singleton runners (#422) * Add support for singleton runners The --reuse-existing flag will cause cml-runner to look up for already existing runners with the specified --labels and skip creating a new one if that's the case. * Deprecate --name with a warning * Apply pre-review suggestions and fixes * Comment out deprecation warning * Remove unnecessary comparison * Rename reuse-existing to reuse * Fix misconception about empty array truthiness * Simplify reuse by name logic * Add missing awaits * Give precedence to name deduplication over label deduplication * Remove commented code * Startup script (#445) * Startup script * lint * snapshots * Add debugging message for #443 (#448) This commit doesn't completely fix #443, but closes it until we can reproduce again the error. * Bitbucket-1000-handling * bump version Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: DavidGOrtega <[email protected]>
This pull request doesn't fix #443 per se and is only intended to facilitate bug reports if the mentioned issue can be reproduced in the future.