RelationalAI / rai-sdk-csharp

The RelationalAI Software Development Kit (SDK) for C#
Apache License 2.0
0 stars 1 forks source link

Fix list engines issue #29

Closed NRHelmi closed 2 years ago

NRHelmi commented 2 years ago

Fix list engines issue Replaces Engine size enumeration with string Closes #26

miazamrai commented 2 years ago

@NRHelmi This will fix the exception but then how would we validate the given size? Enum was a way to accepting only the valid sizes. IMO we should wait for the infra team to come up with the a valid solution for the given issue (https://github.com/RelationalAI/relationalai-infra/issues/3647) before we make any changes in the SDK

NRHelmi commented 2 years ago

@NRHelmi This will fix the exception but then how would we validate the given size? Enum was a way to accepting only the valid sizes. IMO we should wait for the infra team to come up with the a valid solution for the given issue (RelationalAI/relationalai-infra#3647) before we make any changes in the SDK

I still think we need to align this with the julia sdk implementation (https://github.com/RelationalAI/rai-sdk-julia/blob/main/src/api.jl#L209-L213), RAICloud api should be able to validate engine types, I don't see a real reason to keep it sdk level:

julia> create_engine(ctx, "test-engine"; size = "whatever")                                                                                                                                                                                                                    
ERROR: 400 Bad Request                                                                                                                 
{                                                                                                                                                                                                                                                                                  "status": "Bad Request",                                                                                                                                                                                                                                                      "message": "validation error",                                                                                                                                                                                                                                              
   "details": "\"size\" field \"whatever\" is not a valid engine size"                          
}   

Also it doesn't make sense to release a new sdk version just because we have a new added or removed engine type. I have no issue with keeping the current Enum except the fact that it will need extra efforts to maintain :pray:

miazamrai commented 2 years ago

@NRHelmi This will fix the exception but then how would we validate the given size? Enum was a way to accepting only the valid sizes. IMO we should wait for the infra team to come up with the a valid solution for the given issue (RelationalAI/relationalai-infra#3647) before we make any changes in the SDK

I still think we need to align this with the julia sdk implementation (https://github.com/RelationalAI/rai-sdk-julia/blob/main/src/api.jl#L209-L213), RAICloud api should be able to validate engine types, I don't see a real reason to keep it sdk level:

julia> create_engine(ctx, "test-engine"; size = "whatever")                                                                                                                                                                                                                    
ERROR: 400 Bad Request                                                                                                                 
{                                                                                                                                                                                                                                                                                  "status": "Bad Request",                                                                                                                                                                                                                                                      "message": "validation error",                                                                                                                                                                                                                                              
   "details": "\"size\" field \"whatever\" is not a valid engine size"                          
}   

Also it doesn't make sense to release a new sdk version just because we have a new added or removed engine type. I have no issue with keeping the current Enum except the fact that it will need extra efforts to maintain 🙏

@NRHelmi I agree. Java is also a typed language but it is also using string variable for the engine size. So, I think we can do the same for C#. Although XS/2 is a size returned by the API but I don't think XS/2 will work while creating the engine. So, I think that the solution to infra issue https://github.com/RelationalAI/relationalai-infra/issues/3647 should not tied to the SDK solution. One more thing. Can we mention in the change log or somewhere that EngineSize enum has been removed so that clients either fix their code when they upgrade to the new version or delay upgrading if they like to.

NRHelmi commented 2 years ago

Change log update, thanks @miazamrai :pray: Create engine users experience is good for julia sdk, so I suggest experimenting with the current change csharp sdk side, I'll update also the java sdk at somepoint

miazamrai commented 2 years ago

@NRHelmi Are you planning to merge the fix and bump up the release version?