-
Notifications
You must be signed in to change notification settings - Fork 896
[Feature:InstructorUI] Capability / image buildtime error #10839
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
There is an issue with this PR, in that the erroring process calls |
@powe97 should work, please review! |
e2e testing needs at least 2 unused capabilities to work properly, so I added one to do so. |
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.
I found this broke existing autograding examples, such as c_failure_messages and c_malloc_not_allowed. This occurred because "submitty/autograding-default:latest" is set as the default container when "container_image" is not specified, so any config running in jailed_sandbox mode on a machine lacking that image is expected to fail.
Probably good to discuss with Barb what the best fix is, as it will likely involve skipping this check for jailed_sandbox gradeables or editing a lot of existing examples.
I did confirm that it correctly failed to build when missing capabilities or images and correctly succeeded with a valid docker config.
Discussed with Barb. Best fix is to bypass the buildtime image check for non-docker gradables, but we still want to confirm capabilities exist. |
I thought the current goal was to dockerize all configs. Is that incorrect? |
There are some cases of autograding that will run on the machine directly, outside of docker, in the jailed sandbox. E.g., graphics grading that uses the graphics card & display. @DarthNyan is pointing out that we have a default value for the docker container, even though we default the use of docker to false. When use of docker is false, it doesn't make sense to check for the presence of the unused-for-this-gradeable, default docker container. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10839 +/- ##
=========================================
Coverage 20.83% 20.83%
Complexity 9108 9108
=========================================
Files 259 259
Lines 34933 34933
Branches 459 459
=========================================
Hits 7277 7277
Misses 27201 27201
Partials 455 455
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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.
The functionality is good but it still does not pass integration checks. In addition the list of capabilities being checked does not encompass the entire list.
Signed-off-by: Christopher Poon <christopherpoonc@gmail.com>
Signed-off-by: Christopher Poon <christopherpoonc@gmail.com>
Signed-off-by: Christopher Poon <christopherpoonc@gmail.com>
1de3eed
to
e1cda5d
Compare
Works on #10747
Gives an error for gradeables at buildtime if the requested capability / image does not exist in the context of that gradeable.
To test, you can modify an existing config for a development course to either have a capability that does not exist or an image that does not exist and rebuild the gradeable for that course.