Skip to content

save mps features into db#64

Merged
ryanlau merged 17 commits into
mainfrom
mps-fix
Jun 12, 2025
Merged

save mps features into db#64
ryanlau merged 17 commits into
mainfrom
mps-fix

Conversation

@ryanlau

@ryanlau ryanlau commented Apr 9, 2025

Copy link
Copy Markdown
Contributor

title

@ryanlau ryanlau requested review from SreeDan and aesteri April 9, 2025 04:19
@ryanlau ryanlau marked this pull request as ready for review April 12, 2025 22:45
Comment thread cloud_webserver_v2/internal/mps/mps.go Outdated
Comment thread cloud_webserver_v2/internal/mps/mps.go
Comment thread cloud_webserver_v2/internal/mps/mps.go Outdated
Comment thread cloud_webserver_v2/internal/mps/mps.go Outdated
Comment thread cloud_webserver_v2/internal/mps/mps.go Outdated
@ryanlau ryanlau requested a review from SreeDan April 13, 2025 23:41
Comment thread cloud_webserver_v2/internal/mps/mps.go Outdated
push:
branches:
- main
- mps-fix

@SreeDan SreeDan Jun 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need mps-fix post merge?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no, i will remove later

if runModel.MpsRecord == nil {
runModel.MpsRecord = make(map[string]interface{})
}
// mpsMetadata := make(map[string]interface{})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pls remove unused comments

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oh, was gonna ask you abt this actually.

we need to leave the commented code in if we wanted to be able to update MPS records with an HTTP request to the server. i personally don't want to have that functionality but what do you think

}
scripts := strings.Split(scriptsParam, ",")

versionParam := r.URL.Query().Get("version")

@SreeDan SreeDan Jun 2, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is version in this context?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the matlab scripts will all be packaged together into 1 package to be deployed onto MPS. version refers to the package version

@@ -285,6 +285,11 @@ func (h *mcapHandler) ProcessMatlabJob(w http.ResponseWriter, r *http.Request) *
}
scripts := strings.Split(scriptsParam, ",")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if scripts is referring to MATLAB scripts, we should rename it to be more descriptive.

@ryanlau ryanlau Jun 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

dy want me to rename the query param too or just the variable?

Comment thread cloud_webserver_v2/internal/models/vehicle_run_model.go Outdated

switch scriptResult.Type {
case "mat", "image":
// scriptResult.Result = /data/mps_generated/file_name.mat

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: unused comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oh i wanted to give an example of scriptResult.Result for future reference

log.Fatalf("generated file does not exist: %s", mpsGeneratedFileLocation)
}

// copy the generated file to the local s3 cache directory

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can't we just simplify logic by just moving the file withos.Rename?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

bc we are moving files across docker volumes, os.Rename doesn't work so we have to unfortunately copy it over manually

func (m *MatlabClient) pollForResults() {
// Polls the MPS for the result of a job until it is ready
// Once it's ready, it processes the job result and then deletes it off MPS
func (m *MatlabClient) pollForJobResult(mpsJob mpsJob, s3Repo *s3.S3Repository) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We may want to include a timeout here just in case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we can set a timeout on the MPS side.

so if a script doesn't finish by X time, it will cancel the execution and say that it timed out which can be handled here.

Comment thread cloud_webserver_v2/internal/s3/s3_repository.go
@ryanlau ryanlau requested a review from SreeDan June 4, 2025 00:19
@ryanlau ryanlau merged commit 58119a5 into main Jun 12, 2025
1 check passed
@ryanlau ryanlau deleted the mps-fix branch June 12, 2025 23:32
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