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

feat(component,instillmodel): migrate instill model component #777

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

chuang8511
Copy link
Contributor

@chuang8511 chuang8511 commented Oct 24, 2024

Because

  • there are breaking change in instill model

This commit

  • migrate the instill model component
  • migrate the existing pipelines

Note

  • QA in the Linear threads
    • ✅ QAed Migration
    • ✅ QAed Pipeline
  • About Read() & Write(), we will phase them out with Instill Model together, which we will discuss in the future plan.

Copy link

linear bot commented Oct 24, 2024

@chuang8511
Copy link
Contributor Author

Hi @jvallesm

Want to clarify if this test code aligns what you think like we discussed before.

@jvallesm
Copy link
Collaborator

Hi @jvallesm

Want to clarify if this test code aligns what you think like we discussed before.

Yes! That's the approach, exactly. Kudos, as grpc doesn't contain a standard library test package like httptest (perhaps there's something similar that isn't builtin), so replicating it here wasn't trivial and you got exactly what I meant 👍☹️👍

Perhaps if we have more use cases for this we can build later a wrapper to simplify the process of creating the server and defining its responses. One first step would be adding the mock to the generated code in protogen-go, then perhaps add a test helper package in x

@chuang8511 chuang8511 marked this pull request as ready for review October 29, 2024 11:39
@chuang8511 chuang8511 force-pushed the chunhao/ins-6554-instill-model-refactor branch from 73f96e4 to a6f43f4 Compare November 1, 2024 18:28
@chuang8511 chuang8511 marked this pull request as draft November 1, 2024 18:29
@chuang8511 chuang8511 marked this pull request as ready for review November 4, 2024 10:29
@chuang8511
Copy link
Contributor Author

@donch1989
This is what I used now. But, I am not sure if it is same as your imagination.

@donch1989
Copy link
Member

@donch1989 This is what I used now. But, I am not sure if it is same as your imagination.

Yes, I think we can use that for now and refactor it later.

@chuang8511
Copy link
Contributor Author

@donch1989 @jvallesm

Do you have time to review it? Or can we merge it after I rebase it?

@chuang8511
Copy link
Contributor Author

And, for the document part, we will need @GeorgeWilliamStrong to review it. Thank you.

@pinglin pinglin changed the title feat(instillmodel): migrate instill model component feat(component,instillmodel): migrate instill model component Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants