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.

Sign up for GitHub

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

Jump to bottom

Documentation correction of pyvw.py #2336

Merged
merged 16 commits into from Apr 3, 2020
Merged

Documentation correction of pyvw.py #2336

merged 16 commits into from Apr 3, 2020

Conversation

Smit-create
Copy link
Contributor

@Smit-create Smit-create commented Mar 14, 2020 •
edited

Fixes #2267

Description
After changing the documentation style to numpy docstyle in #2305, this PR aims at changing the docs of pyvw.py accordingly.

@Smit-create
Copy link
Contributor Author

@jackgerrits @gramhagen Please review the changes. The updated doc of pyvw.py with this diff is attached here.
pyvwdoc1.pdf

@Smit-create
Copy link
Contributor Author

I have made the suggested changes. Please find the updated doc attached here.
pyvwdoc2.pdf

@Smit-create
Copy link
Contributor Author

@jackgerrits Please review this. Thanks!

@RituRajSingh878
Copy link
Contributor

RituRajSingh878 commented Mar 19, 2020 •
edited

I don't know if you have already check this so if not then
@Smit-create later maybe we will follow doc8 style. one of the checks is-

- lines should not be longer than 79 characters - D001

We can modify this length but Can you please follow this check in the doc.
@jackgerrits We should follow the above check for the line or line length will be greater than this in our doc maybe about 100 char.

@Smit-create
Copy link
Contributor Author

I will have a look at that and wrap the lines within 72 characters following the PEP8 style.

@Smit-create
Copy link
Contributor Author

@jackgerrits @gramhagen I have used numpy doc format of documentation, and PEP8 style to wrap the lines. Please review the changes.

@Smit-create Smit-create changed the title [WIP] Documentation correction of pyvw.py Documentation correction of pyvw.py Mar 21, 2020
@Smit-create Smit-create reopened this Mar 22, 2020
@Smit-create Smit-create mentioned this pull request Mar 25, 2020
Closed
@Smit-create
Copy link
Contributor Author

Please review this.

gramhagen
gramhagen reviewed Mar 25, 2020
View reviewed changes
Copy link
Contributor

@gramhagen gramhagen left a comment

Choose a reason for hiding this comment

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

looks good, have you been able to run any python tests to make sure functionality didn't change?

@Smit-create
Copy link
Contributor Author

have you been able to run any python tests

It passes all the tests locally.

@Smit-create Smit-create reopened this Mar 26, 2020
@jackgerrits
Copy link
Member

Using repr with return the string wrapped in quotes, as it is the representation of that. Passing this to dedent will break the intended functionality of dedent. I really don't see a reason to combine repr and dedent

@Smit-create
Copy link
Contributor Author

Yes, I checked that it didn't work in all cases. I can fix it another way

@Smit-create
Copy link
Contributor Author

Hi @jackgerrits, I have added a new function to resolve this. It works in the following way:

>>> from vowpalwabbit import pyvw
>>> s = """
...     Hello
...             World
... """
>>> pyvw.fillindents(s)
'Hello   World'

@jackgerrits
Copy link
Member

jackgerrits commented Mar 27, 2020 •
edited

What is the issue you are trying to solve here?

If you're just trying to wrap long strings then you just have adjacent string literals. https://stackoverflow.com/questions/3346230/wrap-long-lines-in-python

@Smit-create
Copy link
Contributor Author

What is the issue you are trying to solve here?

In order to display long lines(which here are broken into multiple lines because of length bounds, mostly error messages) as the single line at the user end instead to directly showing these multiple lines with indentation

@jackgerrits
Copy link
Member

Okay, all you need to do is have string literals broken over several lines. They are automatically concatenated. Check out the link.

jackgerrits
jackgerrits reviewed Mar 27, 2020
View reviewed changes
python/vowpalwabbit/pyvw.py Outdated
@@ -2,12 +2,35 @@
"""Python binding for pylibvw class"""

from __future__ import division
import pylibvw
from pylibvw import example as pylibvw_example, vw as pylibvw_vw
Copy link
Member

Choose a reason for hiding this comment

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

You must also import predictor and search.

This is technically changing the public interface of the package, so I am hesitant to merge this atm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest to import it only when needed. It is because it creates a significant time difference in importing the module. Have a look at #2369(comment)

Copy link
Member

Choose a reason for hiding this comment

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

Does this break the public interface? Or is it still possible to do the below?

from vowpalwabbit import pyvw
pyvw.pylibvw.search...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't break the public interface. Just I meant to import required classes from pylibvw. Other functionalities remains the same as they are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahha, I wonder how will pyvw. pylibvw.search work. As there is no such class/function named pylibvw in pyvw. So, I think this should not work presently too. I am not aware if python allows this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this break the public interface? Or is it still possible to do the below?

This won't work even in current master.


>>> from vowpalwabbit import pyvw
>>> pyvw.pylibvw.vw
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 'vowpalwabbit.pyvw' has no attribute 'pylibvw'

@jackgerrits
Copy link
Member

This PR seems to have turned into 2 things:

  • Documentation updates
  • Code/lint fixes

It looks like the original PR was to do with documentation. Can you please open a separate PR for the code changes? It's already a big one and I don't want to complicate this review further

@Smit-create
Copy link
Contributor Author

Sure! I will revert the changes related to code. And keep this limited to documentation. I will wait for this to merge, and then open related to code/lint

@Smit-create
Copy link
Contributor Author

I have limited this to doc fixes. I will be waiting for this to be merged for code fixes

@Smit-create
Copy link
Contributor Author

@jackgerrits Can you please restart Travis CI?

@jackgerrits
Copy link
Member

Travis seems to be having some issues. Can you try pushing an empty commit to retrigger it?

@Smit-create Smit-create reopened this Mar 28, 2020
gramhagen
gramhagen suggested changes Mar 31, 2020
View reviewed changes
Copy link
Contributor

@gramhagen gramhagen left a comment

Choose a reason for hiding this comment

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

small change requested on prediction_type

jackgerrits
jackgerrits reviewed Apr 2, 2020
View reviewed changes
python/vowpalwabbit/pyvw.py Outdated
@@ -155,7 +155,7 @@ def get_prediction(ec, prediction_type):
>>> import pylibvw
>>> vw = pyvw.vw(quiet=True)
>>> ex = vw.example('1 |a two features |b more features here')
>>> pyvw.get_prediction(ex, pylibvw.vw.pSCALAR)
>>> pyvw.get_prediction(ex, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Why are you using an integer here? Please use the enum

@jackgerrits jackgerrits merged commit f3aabc0 into VowpalWabbit:master Apr 3, 2020
@Smit-create
Copy link
Contributor Author

Thanks @jackgerrits

@jackgerrits jackgerrits mentioned this pull request Apr 30, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gramhagen gramhagen gramhagen requested changes

@jackgerrits jackgerrits jackgerrits left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Documentation of pyvw.py
4 participants
@Smit-create @RituRajSingh878 @gramhagen @jackgerrits