-
Notifications
You must be signed in to change notification settings - Fork 4
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
EVA-3414 Store validation result and pass it to submit #14
Conversation
cli/eva_sub_cli.py
Outdated
config_file_path = os.path.join(args.submission_dir, SUB_CLI_CONFIG_FILE) | ||
sub_config = WritableConfig(config_file_path, version=__version__) | ||
|
||
if args.task == RESUME: |
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.
Optional: From a design standpoint, if validation fails for a submission, I feel that issuing "resume" at that point should resume the validation. #standup
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 think I'd agree. We might want to discuss what is and what isn't resumable but if we have a generic resume option it should apply to all steps.
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.
This being said implementing a comprehensive resume strategy seems outside of the scope of this ticket.
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.
Tried to simplify resume option
cli/eva_sub_cli.py
Outdated
config_file_path = os.path.join(args.submission_dir, SUB_CLI_CONFIG_FILE) | ||
sub_config = WritableConfig(config_file_path, version=__version__) | ||
|
||
if args.task == RESUME: |
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 think I'd agree. We might want to discuss what is and what isn't resumable but if we have a generic resume option it should apply to all steps.
cli/eva_sub_cli.py
Outdated
if not args.vcf_files_mapping: | ||
raise Exception(f"Please provide a CSV file which provides VCF and corresponding assembly information " | ||
f"(use the --vcf_files_mapping switch)") | ||
if not args.metadata_json and not args.metadata_xlsx: | ||
raise Exception(f"Please provide the file that describes the project, analysis, samples and files " | ||
f"using either --metadata_json or --metadata_xlsx") |
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.
This could probably be improved in a subsequent ticket but the upload does need to know where the VCF and metadata is.
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.
updated
cli/eva_sub_cli.py
Outdated
config_file_path = os.path.join(args.submission_dir, SUB_CLI_CONFIG_FILE) | ||
sub_config = WritableConfig(config_file_path, version=__version__) | ||
|
||
if args.task == RESUME: |
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.
This being said implementing a comprehensive resume strategy seems outside of the scope of this ticket.
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 think these additions would be better suited to reporter.py
so they apply to all subsequent validators we might implement.
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.
changed
cli/docker_validator.py
Outdated
def update_config_with_validation_result(self): | ||
self.sub_config.set(VALIDATION_RESULTS, value=self.results) | ||
self.sub_config.set(READY_FOR_SUBMISSION_TO_EVA, value=self.verify_ready_for_submission_to_eva()) | ||
self.sub_config.write() |
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 think I would model the writing of the config to the way it works in ELOAD
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.
updated
SUBMIT = 'submit' | ||
RESUME = 'resume' | ||
|
||
logging_config.add_stdout_handler() |
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 call to add_stdout_handler
should be made in the main
function. This avoids having multiple handlers being added if this file is ever loaded as a library.
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.
updated
9499d91
to
8d84d55
Compare
No description provided.