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

Document magic data loading in TextClassificationProcessor PR #383

Merged
merged 10 commits into from Jun 3, 2020

Conversation

PhilipMay
Copy link
Contributor

@PhilipMay PhilipMay commented May 29, 2020

This is the PR for #378

TODO

  • fix stupid CI problem

@PhilipMay
Copy link
Contributor Author

PhilipMay commented May 30, 2020

The CI problem seems to come from this issue: #382 (comment)

It is fixed here: 4109389

If you merge this PR: #382 I could just rebase master and this CI error is also fixed.

@PhilipMay
Copy link
Contributor Author

Rebased Master

@PhilipMay
Copy link
Contributor Author

CI server seems to be broken: ##[error]We stopped hearing from agent Hosted Agent. Verify the agent machine is running and has a healthy network connection. Anything that terminates an agent process, starves it for CPU, or blocks its network access can cause this error. For more information, see: https://go.microsoft.com/fwlink/?linkid=846610

Should I do a fake commit to restart...?

@Timoeller Timoeller self-requested a review May 31, 2020 13:36
Copy link
Contributor

@Timoeller Timoeller left a comment

Choose a reason for hiding this comment

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

Thanks for the docstring improvements. They make data loading more transparent for the user.

General comment: could you also add the docstring to the other processors, not just TCP? I can also gladly do that when this PR is merged.

This CI error seems really strange. I made a small comment for a proposed change in the TCP docstring - please try another commit with the proposed changes. Hopefully the CI runs through.

farm/data_handler/processor.py Outdated Show resolved Hide resolved
@PhilipMay
Copy link
Contributor Author

General comment: could you also add the docstring to the other processors, not just TCP? I can also gladly do that when this PR is merged.

I did that. Hope I did not miss anything.

@PhilipMay PhilipMay requested a review from Timoeller June 1, 2020 20:04
@PhilipMay
Copy link
Contributor Author

I think I am done with this. Can you please review?
Thanks! :-)

Copy link
Contributor

@Timoeller Timoeller left a comment

Choose a reason for hiding this comment

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

Looking very well.

Thanks for also adjusting to the proposed changes.

@Timoeller Timoeller merged commit 2966339 into deepset-ai:master Jun 3, 2020
@PhilipMay PhilipMay deleted the tcp-magic-doc branch June 8, 2020 19:46
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

2 participants