Add Pathlib support for 'apply_effects_file' #1048
Conversation
|
@vincentqb , The newly added testcases |
|
Thanks for catching this: unfortunately, jit does not support Union type, pytorch/pytorch#41412. This means we won't be able to support path also with this function here until pytorch/pytorch#41412 is resolved. Thanks for opening this pull request, we can keep it open in case pytorch/pytorch#41412 gets resolved and we would then be able to merge it even though this may take some time. |
vincentqb
left a comment
vincentqb
left a comment
There was a problem hiding this comment.
I'll mark this as "changed needed" until pytorch/pytorch#41412 is resolved, thanks for opening this and noticing the issue :)
| load_params("sox_effect_test_args.json"), | ||
| name_func=lambda f, i, p: f'{f.__name__}_{i}_{p.args[0]["effects"][0][0]}', | ||
| ) | ||
| def test_apply_effects_path(self, args): |
There was a problem hiding this comment.
This is overkill for testing the Pathlib.Path compatibility. Can you just pick up one test case, so that the focus of the test is intuitively clear that it's about Pathlib.Path.
There was a problem hiding this comment.
@mthrok changed the pathlib unit test by hard coding a specific test case argument
| @_mod_utils.requires_module('torchaudio._torchaudio') | ||
| def apply_effects_file( | ||
| path: str, | ||
| path: Union[str, Path], |
There was a problem hiding this comment.
You can remove the Union and just use str, but explaining why in the docstring which is how it is done in sox_io.
There was a problem hiding this comment.
I second this, and we should be able to see if the command is jitable :)
There was a problem hiding this comment.
@vincentqb changed type as per sox_io.
Also used str() like sox_io to convert Path to string, as os.fspath() is not supported by torchscript.
Now all checks passed.
Co-authored-by: holly1238 <[email protected]>
typo infinitely
4 participants
Relates to #950
For unit test copied the test for , I copied the existing one and passed Path object .
Didn't do any factoring as it fetching parameters of test case