Skip to content

Conversation

@timlod
Copy link
Contributor

@timlod timlod commented Sep 13, 2020

Allows for passing of pathlib.Path filepath to sox_io functions.
See #902.

For type union docstrings I used (str/pathlib.Path), as I found this pattern was used in the kaldi_io methods (for str/FileDescriptor).
There may be a better pattern though?

Copy link
Contributor

@vincentqb vincentqb 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 following-up on comment :)

torchscript tests are failing, and indicate that the union type is not supported by torchscript, see also here. For soundfile, even though functions convert to string and make the code compatible with path, only the string type is in the signature, here. I would be ok having a similar unofficial support for pathlib as is done in soundfile here.

Copy link
Contributor

@mthrok mthrok left a comment

Choose a reason for hiding this comment

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

Hi @timlod

Thanks for the PR. The signature part is a tricky one, which had slipped from my mind when I was replying in #902.

The "sox_io" backend is designed to be compatible with TorchScript compiler (so that the function works in C++ application too), so types that are specific to Python (in this case, they are Union, Path) cannot be added. Also, because of the way the loading function (in C++) is bound to Python code, we cannot pass file-like object, even though that's common practice in Python.

I checked the intermediate representation that TorchScript compiler generates and filepath = str(filepath) has no effect in TorchScript so this change is good. So just keeping the signature as is should do.

Copy link
Contributor

@mthrok mthrok left a comment

Choose a reason for hiding this comment

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

Hi @timlod

Thanks for updating the PR. Changes related to the other backends can be omitted from this PR.

Copy link
Contributor

@mthrok mthrok 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, thanks!

@mthrok mthrok merged commit e7161ac into pytorch:master Sep 18, 2020
@vincentqb vincentqb mentioned this pull request Oct 13, 2020
16 tasks
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.

3 participants