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

Use warnings.warn instead of logger.warning #321

Merged
merged 10 commits into from
May 29, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 108 additions & 3 deletions README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -255,9 +255,114 @@ Since going over all (or select) keys in an S3 bucket is a very common operation
>>> prefix = 'annual/monthly_rain/'
>>> for key, content in s3_iter_bucket(bucket, prefix=prefix, accept_key=lambda key: '/201' in key, workers=1, key_limit=3):
... print(key, round(len(content) / 2**20))
annual/monthly_rain/2010.monthly_rain.nc 14
annual/monthly_rain/2011.monthly_rain.nc 14
annual/monthly_rain/2012.monthly_rain.nc 14
annual/monthly_rain/2010.monthly_rain.nc 13
annual/monthly_rain/2011.monthly_rain.nc 13
annual/monthly_rain/2012.monthly_rain.nc 13


Migrating to the New `open` Function
Copy link
Owner

@piskvorky piskvorky May 31, 2019

Choose a reason for hiding this comment

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

@mpenkov what happened here? I remember commenting on this line in a review, and you fixing it. But now it's back in the master README.

------------------------------------

Since 1.8.1, there is a `smart_open.open` function that replaces `smart_open.smart_open`.
The new function offers several advantages over the old one:

- 100% compatible with the built-in `open` function (aka `io.open`): it accepts all
the parameters that the built-in `open` accepts.
- The default open mode is now "r", the same as for the built-in `open`.
The default for the old `smart_open.smart_open` function used to be "rb".
- Fully documented keyword parameters (try `help("smart_open.open")`)

The instructions below will help you migrate to the new function painlessly.

First, update your imports:

.. code-block:: python

>>> from smart_open import smart_open # before
>>> from smart_open import open # after

In general, `smart_open` uses `io.open` directly, where possible, so if your
code already uses `open` for local file I/O, then it will continue to work.
If you want to continue using the built-in `open` function for e.g. debugging,
then you can `import smart_open` and use `smart_open.open`.

**The default read mode is now "r" (read text) by default.**
If your code was implicitly relying on the default mode being "rb" (read
binary), then you'll need to update it and pass "r" explicitly.

Before:

.. code-block:: python

>>> buf = b''
>>> buf += smart_open('s3://commoncrawl/robots.txt').read(32)

After:

.. code-block:: python

>>> buf = b''
>>> buf += open('s3://commoncrawl/robots.txt', 'rb').read(32)

The `ignore_extension` keyword parameter is now called `ignore_ext`.
It behaves identically otherwise.

The most significant change is in the handling on keyword parameters for the
transport layer, e.g. HTTP, S3, etc. The old function accepted these directly:

.. code-block:: python

>>> url = 's3://smart-open-py37-benchmark-results/test.txt'
>>> session = boto3.Session(profile_name='smart_open')
>>> smart_open(url, 'r', session=session).read(32)
'first line\nsecond line\nthird lin'

The new function accepts a `transport_params` keyword argument. It's a dict.
Put your transport parameters in that dictionary.

.. code-block:: python

>>> url = 's3://smart-open-py37-benchmark-results/test.txt'
>>> session = boto3.Session(profile_name='smart_open')
>>> params = {'session': session}
>>> open(url, 'r', transport_params=params).read(32)
'first line\nsecond line\nthird lin'

Renamed parameters:

- `s3_upload` ➡ `multipart_upload_kwargs`
- `s3_session` ➡ `session`

Removed parameters:

- `profile_name`

**The `profile_name` parameter has been removed.**
Copy link
Owner

Choose a reason for hiding this comment

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

Something is wrong with the backticks. They have no effect in this section (look like normal text when rendered), and this line actually renders them as literal backticks:

Screen Shot 2019-05-31 at 09 58 03

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that's just the way github renders RST. I don't know what we can do about that.

Copy link
Owner

@piskvorky piskvorky May 31, 2019

Choose a reason for hiding this comment

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

Aaah, this is RST, not Markdown! That explains it.

Literal blocks in RST use double-backticks IIRC, not single-backtick.

Do we need to use RST though? MD easier? Or is it something to do with PyPI relying on RST…

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it's because of PyPI

Pass an entire boto3.Session object instead.

Before:

.. code-block:: python

>>> url = 's3://smart-open-py37-benchmark-results/test.txt'
>>> smart_open(url, 'r', profile_name='smart_open').read(32)
'first line\nsecond line\nthird lin'

After:

.. code-block:: python

>>> url = 's3://smart-open-py37-benchmark-results/test.txt'
>>> session = boto3.Session(profile_name='smart_open')
>>> params = {'session': session}
>>> open(url, 'r', transport_params=params).read(32)
'first line\nsecond line\nthird lin'

See `help("smart_open.open")` for the full list of acceptable parameter names,
or view the help online `here <https://github.com/RaRe-Technologies/smart_open/blob/master/help.txt>`__.

If you pass an invalid parameter names, the `open` function will warn you about it.
Copy link
Owner

@piskvorky piskvorky May 31, 2019

Choose a reason for hiding this comment

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

typo:

If you pass an invalid parameter name, the `open` function will warn you about it.

Keep an eye on your logs for WARNING messages from smart_open.

Comments, bug reports
---------------------
Expand Down
12 changes: 12 additions & 0 deletions release/prepare.sh
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ set +u # work around virtualenv awkwardness
source sandbox.venv/bin/activate
set -u

python -m doctest ../README.rst

cd ..
pip install -e .[test] # for smart_open
pip install .[test] # for gensim
Expand All @@ -56,6 +58,16 @@ read -p "Press Enter to continue..."
$EDITOR CHANGELOG.md
git commit CHANGELOG.md -m "updated CHANGELOG.md for version $version"

python -c 'help("smart_open")' > help.txt

#
# The below command will fail if there are no changes to help.txt.
# We can safely ignore that failure.
#
set +e
git commit help.txt -m "updated help.txt for version $version"
set -e

echo "Have a look at the current branch, and if all looks good, run merge.sh"

cd "$script_dir"
Expand Down
18 changes: 16 additions & 2 deletions smart_open/smart_open_lib.py
Original file line number Diff line number Diff line change
Expand Up @@ -380,9 +380,23 @@ def open(
}


_MIGRATION_NOTES_URL = (
'https://github.com/RaRe-Technologies/smart_open/blob/master/README.rst'
'#migrating-to-the-new-open-function'
)


def smart_open(uri, mode="rb", **kw):
"""Deprecated, use smart_open.open instead."""
logger.warning('this function is deprecated, use smart_open.open instead')
"""Deprecated, use smart_open.open instead.

See the migration instructions: %s

""" % _MIGRATION_NOTES_URL

warnings.warn(
'This function is deprecated, use smart_open.open instead. '
'See the migration notes for details: %s' % _MIGRATION_NOTES_URL
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
'See the migration notes for details: %s' % _MIGRATION_NOTES_URL
'See the migration instructions: %s' % _MIGRATION_NOTES_URL

)

#
# The new function uses a shorter name for this parameter, handle it separately.
Expand Down