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

[AiLab] Delete controller method, route and tests for ML models #39844

Merged
merged 8 commits into from
Apr 8, 2021

Conversation

Erin007
Copy link
Contributor

@Erin007 Erin007 commented Mar 31, 2021

Setting up the controller action and route to delete trained models from the UserMlModel table and S3, including a couple of tests.

@Erin007 Erin007 requested review from breville, made-line and a team March 31, 2021 20:34
return head :forbidden unless @user_ml_model.user_id == current_user.id
@user_ml_model.destroy
delete_from_s3(@user_ml_model.model_id)
end
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to specify anything about what we return as content here? dont_cache, no_content, etc.?

Comment on lines +74 to +76
def delete_from_s3(model_id)
AWS::S3.delete_from_bucket(S3_BUCKET, model_id)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Great work! This is new to me. Would we ever need to verify/test that this action is successful in S3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This delete method will return a boolean indicating whether the deletion worked or not. I took your idea and use it in the check above. Thanks!

@Erin007 Erin007 requested a review from a team April 7, 2021 17:48
get 'ml_models/names', to: 'ml_models#user_ml_model_names'
get 'ml_models/:model_id', to: 'ml_models#get_trained_model'
get 'ml_models/:model_id/metadata', to: 'ml_models#user_ml_model_metadata'
resources :ml_models, only: [:show, :destroy] do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@madelynkasula You suggested I use resource for the ml_models routes on an older PR - just now getting to it. Is this what you were thinking?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this is exactly what i was thinking! thank you for following up on that

Comment on lines +61 to +62
render json: {id: @user_ml_model.id, status: status}
head :no_content
Copy link
Member

Choose a reason for hiding this comment

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

These appear to conflict with other, but so long as #39924 takes care of it, then I guess that's okay.

@Erin007 Erin007 merged commit ac70118 into staging Apr 8, 2021
@Erin007 Erin007 deleted the delete-models branch April 8, 2021 21:58
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.

None yet

4 participants