Skip to content

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
@donch1989 donch1989 marked this pull request as draft January 13, 2025 08:44
@pinglin pinglin force-pushed the main branch 5 times, most recently from 4357e3d to 61573ac Compare June 29, 2025 10:11
@pinglin pinglin force-pushed the main branch 5 times, most recently from 91eb182 to 349ccd3 Compare July 5, 2025 01:09
@pinglin pinglin force-pushed the main branch 2 times, most recently from 21e92d2 to 34bb299 Compare July 15, 2025 20:41
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