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

Fix: send override fields #19

Merged
merged 32 commits into from
Jan 9, 2025
Merged

Fix: send override fields #19

merged 32 commits into from
Jan 9, 2025

Conversation

madcampos
Copy link

Add the changes field to the execute request as well as the convert request send after executing the model.

Questions still to be answered:

  • Should we cleanup the changes after uploading or executing the model?
  • Is it needed to send the changes on both convert and execute?

@odjuricicTT
Copy link

@madcampos

Should we cleanup the changes after uploading or executing the model?

Hmm, need to think about this. One option is to cleanup every time and hold this state on the backend. What do we do currently with optimization policy? And do we keep any other state on the frontend?

Is it needed to send the changes on both convert and execute?

I think that just execute is needed.

@madcampos
Copy link
Author

@odjuricicTT we don't keep any other state on the frontend, just the changes to fields and the optimization policy.

For optimization policy, we don't cleanup the value. But I think for overwritten fields it makes sense to cleanup changes as those would be applied when the model is executed, no?

@madcampos madcampos marked this pull request as ready for review January 8, 2025 22:29
@madcampos madcampos merged commit 982ffed into main Jan 9, 2025
2 checks passed
@madcampos madcampos deleted the fix/send-override-fields branch January 9, 2025 20:28
@madcampos madcampos restored the fix/send-override-fields branch January 9, 2025 21:27
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.

2 participants