Skip to content

Conversation

miazamrai
Copy link
Contributor

@miazamrai miazamrai commented Nov 3, 2022

Moved the test code to be called as github action so that the source for testing via the python sdk resides on a single place. It will allow the consumers to absorb any changes made to the test logic.

@miazamrai miazamrai requested review from NRHelmi and larf311 November 4, 2022 07:01
@miazamrai miazamrai marked this pull request as ready for review November 4, 2022 07:02
@miazamrai
Copy link
Contributor Author

@larf311 @NRHelmi We need to do same kind of stuff for all the other SDKs, that is, moving the test code to a github action. It would be nice that we use the same folder github folder structure for all the SDKs.

@NRHelmi
Copy link
Contributor

NRHelmi commented Nov 4, 2022

@miazamrai so we don't need to redefine action.yml in raicode right ?

@miazamrai
Copy link
Contributor Author

@miazamrai so we don't need to redefine action.yml in raicode right ?

Yes, the other repository workflow will checkout the sdk and directly run the action from the sdk’s action folder.

@larf311
Copy link

larf311 commented Nov 4, 2022

Don't you need to (optionally) pass in the engine version?

Copy link
Contributor

@NRHelmi NRHelmi left a comment

Choose a reason for hiding this comment

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

thanks @miazamrai

@miazamrai
Copy link
Contributor Author

miazamrai commented Nov 4, 2022

Don't you need to (optionally) pass in the engine version?

it will be like this.
https://github.com/RelationalAI/raicode/blob/276b5325acb9daca2d453903f3712c838a228fd9/.github/workflows/raicloud-bench-sdktest-all.yml#L27

@larf311
Copy link

larf311 commented Nov 4, 2022

Is there a reason not to make the engine version (or perhaps custom headers) an explicit input? It would make the action self documenting. As it is now, callers of this action need to know the special environment variable to set.

@miazamrai
Copy link
Contributor Author

Is there a reason not to make the engine version (or perhaps custom headers) an explicit input? It would make the action self documenting. As it is now, callers of this action need to know the special environment variable to set.

I opted the env variable approach for the following reasons,

  1. The custom headers are treated by the SDKs as env var.
  2. Same github action is being used by both the SDK's CI build and the other repositories and it is not the requirement for the SDK's CI.
  3. Even if the engine version is sent as an optional input param, the action needs to export it as an env var with the right key/value pair.
  4. Wanted to keep the engine version details (header details) internal to the rai private repository because we don't want to expose the internal information to public since SDK is a public repo.

@miazamrai
Copy link
Contributor Author

@larf311 If you are agree with the current approach then I will merge this to main.

@larf311
Copy link

larf311 commented Nov 4, 2022

  1. Why does a caller of the action need to know if internally it uses env variables or command line params or a config file?
  2. That's why it would be an optional input
  3. I don't really understand this argument.
  4. You could make the input be custom headers instead of engine version to avoid this, which is probably more robust since it will allow additional functionality such as log levels, etc.

@miazamrai
Copy link
Contributor Author

  • I don't really understand this argument.

I like #4

@miazamrai
Copy link
Contributor Author

@larf311 Added the custom headers input param.

@miazamrai miazamrai merged commit 4142794 into main Nov 4, 2022
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.

3 participants