rh-aiservices-bu / fraud-detection

https://rh-aiservices-bu.github.io/fraud-detection/
Apache License 2.0
12 stars 22 forks source link

rhoai-4386 model serving and ui updates for 2.8 #25

Closed MelissaFlinn closed 3 months ago

MelissaFlinn commented 3 months ago

updating the Fraud detection tutorial files to address model serving and UI updates in RHOAI 2.8 see https://issues.redhat.com/browse/RHOAIENG-4386 for details

erwangranger commented 3 months ago

@MelissaFlinn , it's a bit hard to read through the changes without a rendered version, but this is a great update.

I do wonder however about the model serving. I'd expect that Single and Multi would end up creating API endpoints that are a bit different in the way their URLs are crafted and accessed. I looked in Notebook 3, but there only seem to be one request here, for the Multi model serving. We may need to update this too, to reflect how to query a model deployed in the single-model version.

This should not delay the current effort, though. It can be done later. Thanks for this.

MelissaFlinn commented 3 months ago

@Erwan - should I remove the module for single-model serving for now? I believe that Chris Chase is addressing that in his test drive update.

erwangranger commented 3 months ago

I think having both options is great to have. People can choose which one they want. But the testing/validation of the endpoint will be slightly different for each, so we'll just need to add a matching notebook. we can do

to differentiate which notebook to use based on which way you chose to serve the model.

MelissaFlinn commented 3 months ago

OK - For single-model serving on Chris's cluster, I deployed the model twice - once with the models/fraud path and the other with the 
models/fraud/1/model.onnx path - both were successful - so I went with the latter one in the instructions. Not sure if that was the correct decision (Chris's test drive uses the models/fraud path). He is on PTO and I did not want to pester him today wdyt?

erwangranger commented 3 months ago

I think either is fine. The fuller path is probably more explicit and will feel less like magic than the shorter path.

I'm still not sure that removing the "frozen" leading slash from our ui was the right move, because now, when I have to fill out this box, I never know if I should enter:

and which ones are likely to work/fail.

If

actually works, it might be the most clear, complete descriptive an unambiguous way of doing it.

MelissaFlinn commented 3 months ago

I just did a quick test, I can successfully deploy a model with the path: /models/fraud/1/model.onnx (single-model serving)

MelissaFlinn commented 3 months ago

ready for merging if approved

cfchase commented 3 months ago

I think either is fine. The fuller path is probably more explicit and will feel less like magic than the shorter path.

I'm still not sure that removing the "frozen" leading slash from our ui was the right move, because now, when I have to fill out this box, I never know if I should enter:

  • models/fraud/1/model.onnx
  • /models/fraud/1/model.onnx
  • /models/fraud/
  • /models/fraud
  • models/fraud

and which ones are likely to work/fail.

If

  • /models/fraud/1/model.onnx

actually works, it might be the most clear, complete descriptive an unambiguous way of doing it.

The best path to use is always models/fraud/ as it works for both single and multi model serving.

oh, models/fraud I think also works

cfchase commented 3 months ago

I think having both options is great to have. People can choose which one they want. But the testing/validation of the endpoint will be slightly different for each, so we'll just need to add a matching notebook. we can do

  • rest_requests_multi-model.ipynb
  • rest_requests_single-model.ipynb

to differentiate which notebook to use based on which way you chose to serve the model.

I agree, we should leave the old notebooks with proper naming for multi-model and add a new one for single model.

MelissaFlinn commented 3 months ago

@erwangranger are you OK with this PR now - can we merge it?

erwangranger commented 3 months ago

I won't have time to do the actual test, but yes, feel free to merge this in.