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

Add command to display the stderr data from submission in terminal #252

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add command to display the stderr data from submission in terminal #252

wants to merge 7 commits into from

Conversation

hkmatsumoto
Copy link
Contributor

The corresponding GCI task

Changes:

  • Add a subcommand to show the content of stderr file of the specified submission, invoked by evalai submission SUBMISSION_ID stderr.
  • Add a helper function of the subcommand.
  • Add tests for the helper function.
  • Add an example of the subcommand to documentation.

CC: @vkartik97 utsav @Ram81 @RishabhJain2018

@hkmatsumoto
Copy link
Contributor Author

hkmatsumoto commented Jan 10, 2020

Since the content of stderr files are basically error messages, it is hard to distinguish stderr messages (i,e: intended behavior) from cli error messages (i,e: unexpected error) if we print the content directly like the figure below.
A

To convince users that the output is a stderr message and cli is not crushing, I decided to print an extra message like the figure below.
B

@hkmatsumoto
Copy link
Contributor Author

And the content of stderr file is this: STDERR

@hkmatsumoto
Copy link
Contributor Author

Hi @vkartik97, thanks for replying on the task page but I still can't find your review. Could you recomment it please?

@krtkvrm
Copy link
Member

krtkvrm commented Jan 11, 2020

Hi,
Can you see this comment: Cloud-CV/EvalAI-ngx#261 (comment)?

@hkmatsumoto
Copy link
Contributor Author

Yes

@krtkvrm
Copy link
Member

krtkvrm commented Jan 11, 2020

The comment is in pending status, so pasting here again:
Why have you made a util: display_submission_stderr, which is only used by stderr, i.e stderr function in submissions.py. I am not able to determine making extracting out the not so large amount statements to a separate util function.
Please let me know if I am missing something.

@hkmatsumoto
Copy link
Contributor Author

hkmatsumoto commented Jan 11, 2020

@vkartik97 That is because I tried to make stderr command similar to other functions including submission and result command.
submission calls display_submission_details and result calls display_submission_result.

@hkmatsumoto
Copy link
Contributor Author

@vkartik97 I personally feel the same thing to you, so I am willing to make another PR that takes in the code of the functions in utils to the command function and wait for opinions from mentors including you.

@krtkvrm
Copy link
Member

krtkvrm commented Jan 11, 2020

If you are going ahead for making PR as commented by you, can you only try for stderr(this PR) and result first?

@hkmatsumoto
Copy link
Contributor Author

Sure but can I ask why?

@krtkvrm
Copy link
Member

krtkvrm commented Jan 11, 2020

The refactoring will be less as compared to going for both result and submission. Once we are satisfied with the changes in result, you can go ahead. And pardon me, I didn't state the reason initially.

@hkmatsumoto
Copy link
Contributor Author

@vkartik97 I got it. Created #253.

@hkmatsumoto
Copy link
Contributor Author

I fell good about the change made in #253. What do you think @vkartik97?

@hkmatsumoto
Copy link
Contributor Author

@Ram81 @RishabhJain2018 Could you also take a look?

@hkmatsumoto
Copy link
Contributor Author

I closed by accident

@hkmatsumoto hkmatsumoto reopened this Jan 12, 2020
@hkmatsumoto
Copy link
Contributor Author

Apart from #253, what do you think of the changes made in this PR?

@krtkvrm
Copy link
Member

krtkvrm commented Jan 12, 2020

The PR for #253 seems good, but is of independent of this task. So, approving this, as refactoring can be a new task altogether.

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