-
Notifications
You must be signed in to change notification settings - Fork 29
Adjust inference and training job names to worker #8524
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
Conversation
📝 WalkthroughWalkthroughThis change updates the naming and handling of custom model training and inference jobs across both backend and frontend code to align with new worker naming conventions. It introduces new job command values ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@MichaelBuessemeyer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing the renaming so fast 🚀.
To get the worker automatically running with the new commands, dbtools.js
needs to be adjusted. As I did this for testing anyway, I took the freedom to already push these changes. Please double check them 🙏
Moreover, I found a little bug where the legacy named job is not displayed correctly. Please find the comment already including a fix suggestion below.
Oh and forgot to tell about the testing: Testing worked fine except for bug noted. |
LGTM! at least from a logical perspective without knowing much about dbtools.js. it was also working when I tested it, so the jobs "started" (not really in dev setup but put to pending) immediately after running |
@MichaelBuessemeyer thank you for the fast review! I incorporated the changes and tested it again. |
@knollengewaechs thanks for the renaming :) could you change the parameter naming in app/controllers/AiModelController.scala lines 192-193? Should now be:
Not sure if other files need to be changed as well when renaming this, but that worked for me with local wk and vx |
@MatthisCl thank you for testing and noticing this! I renamed the parameters.
I am also not 100% sure, but I couldnt find any meaningful occurences of |
Please ignore that 🙈 |
Oh damn. I misunderstood the changes. I thought |
thanks for having a look! 🙏 @MichaelBuessemeyer So I think we might be good then :) |
Oh sorry, I didn't see the newest comment 🙈 Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for incoorperating the feedback and sorry for taking so much time until re-review from my side.
Everything except on small open TODO (see my comment) looks good and the code works great. The open TODO is only about a little cleaner naming in the code so imo no re-review should be necessary to not block this pr any longer :D
Thanks @MichaelBuessemeyer and @knollengewaechs. I addressed the last feedback and if everything is green I'll merge this later today :) |
URL of deployed dev instance (used for testing):
https://adjustinferenceandtrainingjobnames.webknossos.xyz/Changes
for training or inferences, this PR
infer_with_model
toinfer_neurons
train_model
totrain_neuron_model
it does not get rid of all occurrences in order to render past jobs with the given names correctly.
Steps to test:
train_model
totrain_neuron_model
insupportedJobCommands
. I assume this is done by the worker or from voxelytics usuallySUCCESS
to see whether the results are shown correctly. add sth like{resultLink:"test"}
asreturnValue
of ainfer_neurons
job with a custom modelTODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)