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

fix(javascript): update Predict models error spec #980

Merged
merged 5 commits into from
Sep 1, 2022

Conversation

francoischalifour
Copy link
Member

🧭 What and Why

🎟 JIRA Ticket: PRED-544

Changes included

This adapts the new error format of the Predict backend when a model is requested but that it's not available.

Here's the new response format:

{
  "user": "user_1",
  "predictions": {
     "funnel_stage": {
        "error": "The funnel_stage prediction is not available."
     }

  }
}

Changes:

  • Added the new optional error key in each model
  • Made value and lastUpdatedAt optional now, because they may not exist if there's an error

Maybe that we could use smarter conditional types to infer that value and lastUpdatedAt do exist when there's no error.

🧪 Test

None added.

@netlify
Copy link

netlify bot commented Sep 1, 2022

Deploy Preview for api-clients-automation ready!

Name Link
🔨 Latest commit f726d3c
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/6310afbf13cb710008e98571
😎 Deploy Preview https://deploy-preview-980--api-clients-automation.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@algolia-bot
Copy link
Collaborator

algolia-bot commented Sep 1, 2022

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.
You can still access the code generated on main via this commit.

millotp
millotp previously approved these changes Sep 1, 2022
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

Looks good,
To keep the required it could be done with a oneOf, however it would also create intermediary types that are not very friendly, this works fine for me

@shortcuts
Copy link
Member

Yep I'd personally go with a oneOf that does something like

funnel_stage:
  oneOf:
    - '#/funnelStage'
    - '#/error'

funnelStage:
  type: object
  description: Prediction for the **funnel_stage** model.
  properties:
    value:
      type: array
      items:
        title: funnel_stage
        type: object
        properties:
          name:
            type: string
          probability:
            type: number
            format: double
            minimum: 0
            maximum: 1
        required:
          - name
          - probability
    lastUpdatedAt:
      type: string
  required:
    - value
    - lastUpdatedAt

error:
  type: object
  description: The error when the requested model is not available
  properties:
    error:
      type: string

And similar for other objects to re-use the error thing

bengreenbank
bengreenbank previously approved these changes Sep 1, 2022
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

tbh I did not knew my suggestion would work gg

It seems that there's duplicate names (ef67c7e#diff-ff4658ef4480ca1020bc6ddcd1e8b6fe043d35d4d28f18d5831f9df2dc53bb66R9), you might need to find an other name

Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Looks great!

@shortcuts shortcuts merged commit 14d2225 into main Sep 1, 2022
@shortcuts shortcuts deleted the fix/predict-error-spec branch September 1, 2022 13:26
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.

None yet

5 participants