Skip to content

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

Open
hkmatsumoto wants to merge 7 commits into
Cloud-CV:masterfrom
hkmatsumoto:add-command-to-display-the-stderr-data-from-submission-in-terminal
Open

Add command to display the stderr data from submission in terminal#252
hkmatsumoto wants to merge 7 commits into
Cloud-CV:masterfrom
hkmatsumoto:add-command-to-display-the-stderr-data-from-submission-in-terminal

Conversation

@hkmatsumoto

Copy link
Copy Markdown
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

hkmatsumoto commented Jan 10, 2020

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor Author

And the content of stderr file is this: STDERR

@hkmatsumoto

Copy link
Copy Markdown
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

krtkvrm commented Jan 11, 2020

Copy link
Copy Markdown
Member

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

@hkmatsumoto

Copy link
Copy Markdown
Contributor Author

Yes

@krtkvrm

krtkvrm commented Jan 11, 2020

Copy link
Copy Markdown
Member

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

hkmatsumoto commented Jan 11, 2020

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
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

krtkvrm commented Jan 11, 2020

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

Sure but can I ask why?

@krtkvrm

krtkvrm commented Jan 11, 2020

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor Author

@vkartik97 I got it. Created #253.

@hkmatsumoto

Copy link
Copy Markdown
Contributor Author

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

@hkmatsumoto

Copy link
Copy Markdown
Contributor Author

@Ram81 @RishabhJain2018 Could you also take a look?

@hkmatsumoto

Copy link
Copy Markdown
Contributor Author

I closed by accident

@hkmatsumoto hkmatsumoto reopened this Jan 12, 2020
@hkmatsumoto

Copy link
Copy Markdown
Contributor Author

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

@krtkvrm

krtkvrm commented Jan 12, 2020

Copy link
Copy Markdown
Member

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