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

[Feature:Autograding] Automated output and input generation #3882

Merged
merged 92 commits into from Aug 2, 2019

Conversation

weastel
Copy link
Contributor

@weastel weastel commented Jun 19, 2019

Please check if the PR fulfills these requirements:

  • The PR title and message follows our guidelines
  • Tests for the changes have been added/updated (if possible)
  • Documentation has been updated/added if relevant

What is the current behavior?

Presently instructor have to provide both output files and input files for grading.

What is the new behavior?

After this PR instructor can provide give grading with both random output and random inputs. They have to add programs in instructor_solution and write commands for input and output generation in config.json.This will generate both inputs and output at time of autograding

Other information?

  1. Install submitty
  2. run BUILD_development.sh script
  3. Run grades for Cpp simple lab Automated input output

weastel and others added 30 commits May 25, 2019 01:32
@weastel weastel requested review from bmcutler and emaicus July 27, 2019 09:40
Copy link
Contributor

@emaicus emaicus left a comment

Choose a reason for hiding this comment

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

Nearly ready. A few changes requested.

@@ -604,25 +604,25 @@ def grade_from_zip(my_autograding_zip_file,my_submission_zip_file,which_untruste
pattern_copy("random_input_to_runner",["*.txt"],random_input_testcase_folder,testcase_folder,tmp_logs)

# copy compile.out to the current directory
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment isn't accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed compile.out to run.out

@@ -25,16 +25,21 @@ int main(int argc, char *argv[]) {
//If test_case_to_run isn't passed in as a parameter, all testcases are run.
int test_case_to_run = -1;
std::string display_variable = "";

std::string generation_type = "";
system("find . -type f -exec ls -sh {} +");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this?


TCLAP::CmdLine cmd("Submitty's main runner program.", ' ', "0.9");
system("find . -type f -exec ls -sh {} +");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@@ -98,48 +110,101 @@ int main(int argc, char *argv[]) {
continue;
}

std::cout << "========================================================" << std::endl;
std::cout << "TEST #" << i << std::endl;
if ( generation_type == "output" ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The clauses of this if statement are nearly duplicates. Can we either break the duplicated portion into the function or else find a way to reduce lines of nearly duplicated code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added header function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you briefly explain what is the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@emaicus Can you please review it again

@weastel weastel requested a review from emaicus July 30, 2019 16:30
@weastel
Copy link
Contributor Author

weastel commented Jul 31, 2019

@bmcutler Are test running weird on my branch or they are from master?

@bmcutler bmcutler changed the title [SYSADMIN ACTION][Feature] Automated output and input generation [Feature:Autograding] Automated output and input generation Jul 31, 2019
@emaicus
Copy link
Contributor

emaicus commented Jul 31, 2019

Pushed a commit which:

  1. Pulls output generation commands out of the validator and instead moves them to the be next to commands. This new field is called solution_commands.
  2. Makes output generation play more nicely with graphics and networked gradeables.
    1. actions and dispatcher_actions can now be taken against an instructor solution file.
    2. solution_commands is inflated to solution_containers at configure time in the same way that commands is inflated to containers. This allows for input to be generated for networked gradeables.
  3. Patches a bug which was causing stdout to be redirected to a STDOUT.txt file even if the instructor specified their own redirect.

This PR should be ready for review.

Copy link
Member

@bmcutler bmcutler left a comment

Choose a reason for hiding this comment

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

I may jump in and fix some of these myself
Some of these are notes for myself
I'm not ready to merge just yet.

patterns_work_to_random_output = complete_config_obj["autograding"]["work_to_random_output"]
pattern_copy("work_to_random_output", patterns_work_to_random_output, tmp_results, tmp_logs)
except:
with open(os.path.join(tmp_logs,"overall.txt"),'a') as f:
Copy link
Member

Choose a reason for hiding this comment

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

Why hasn't this been resolved?

@@ -466,6 +464,60 @@ def grade_from_zip(my_autograding_zip_file,my_submission_zip_file,which_untruste

os.chdir(tmp_work)

#random_input_tmp_work = os.path.join(tmp_work,"random_input")
Copy link
Member

Choose a reason for hiding this comment

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

remove these newly added, commented out lines

autograder/autograder/grade_item.py Show resolved Hide resolved
cwd=random_input_testcase_folder)
# remove the compilation program
untrusted_grant_rwx_access(which_untrusted, random_input_testcase_folder)
os.remove(os.path.join(random_input_testcase_folder,"my_runner.out"))
Copy link
Member

Choose a reason for hiding this comment

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

The generator code & the solution code are still in this folder, right?
Isn't this a risk that the student can read these files?
(solution code especially, should we require that is placed in a separate config folder)


with open(os.path.join(tmp_logs,"output_generator_log.txt"), 'w') as logfile:
for testcase_num in range(1, len(my_testcases)+1):
testcase_folder = os.path.join(tmp_work_random_output, "test{:02}".format(testcase_num))
Copy link
Member

Choose a reason for hiding this comment

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

again, let's add an "if no need to run output generation continue" here


# copy test input into testcase folder
copy_contents_into(job_id,test_input_path,testcase_folder,tmp_logs)
pattern_copy("random_input_to_runner",["*.txt"],random_input_testcase_folder,testcase_folder,tmp_logs)
Copy link
Member

Choose a reason for hiding this comment

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

What if the random input is not a .txt file?
(I know I said we didn't need to worry about this for version 1.0, so maybe we can ignore this, but let's add a comment at least)
This is preventing us from pull generator.py & solution.py into the test case -- but it's sloppy.

assert(containers.size() > 0);

if (this->CONTAINER_NAME == ""){
//TODO add back in if possible.
Copy link
Member

Choose a reason for hiding this comment

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

In this PR or later?

whole_config["autograding"]["work_to_details"].push_back("test*/*_diff.json");
whole_config["autograding"]["work_to_details"].push_back("**/README.txt");
whole_config["autograding"]["work_to_details"].push_back("input_*.txt");
//todo check up on how this works.
whole_config["autograding"]["work_to_details"].push_back("test*/input_*.txt");
}

if (whole_config["autograding"].find("work_to_random_output") == whole_config["autograding"].end()){
Copy link
Member

Choose a reason for hiding this comment

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

This clause does not make sense to me

@@ -14,9 +14,85 @@
// =====================================================================
// =====================================================================

std::string addRedirectsToCommand(std::string this_command, std::string which){
// Check to see if the instructor is already capturing STDIN
if(this_command.find("1>") == std::string::npos){
Copy link
Member

Choose a reason for hiding this comment

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

I think this (and other find's in this function) is fragile to quoted or escaped characters on the command line.
I'm not sure why so much additional code is needed(?)

Copy link
Member

Choose a reason for hiding this comment

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

Was this totally not implemented before? (it may not have been)

@bmcutler bmcutler merged commit 7e7adf0 into master Aug 2, 2019
@bmcutler bmcutler deleted the automated_input_generator branch August 2, 2019 16:37
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

3 participants