Skip to content

Removing of audio fetching from inference interface - #1268

Closed
stachu86 wants to merge 1 commit into
speechbrain:developfrom
stachu86:load_audio_without_fetching
Closed

stachu86 wants to merge 1 commit into
speechbrain:developfrom
stachu86:load_audio_without_fetching

Conversation

@stachu86

Copy link
Copy Markdown
Contributor

The method load_audio() uses fetch() functionality, which (according to comments) is clearly intended to works with pretrained models, not audio files. This results in bloating working directory with symlinks to (local) audio files during classification, which I believe is a bug.

If this was truly intended, ability to provide savedir shoud be included in speechbrain.pretrained.interfaces.EncoderClassifier.classify_file() instead of my proposed changes.

@Gastron

Gastron commented Jan 24, 2022

Copy link
Copy Markdown
Collaborator

This was added intentionally. Instead, the file methods in the Pretrained interfaces should take the savedir option.

@stachu86

stachu86 commented Jan 24, 2022

Copy link
Copy Markdown
Contributor Author

@Gastron is preventing fetching (and generating symlinks) if savedir is None would be acceptable solution?

@Gastron

Gastron commented Jan 24, 2022

Copy link
Copy Markdown
Collaborator

Perhaps we could instead finally change the fetch behaviour. This has been requested for other reasons, as well, e.g. issue #1055.

We could instead do the following:

  1. In fetch, for local files, create no symlink.
  2. Also in fetch, for external files download/move files to save directory, instead of symlinking.
  3. In Pretrainer, combine collect_files and load_collected into a single method, which internally takes care of distributed cases. If the source is local, all good, if the source is external, only the main process should download the files. We might not require the collect_in directory to be set (accept None but raise error if it is None and source is external).

@KacperKubara

Copy link
Copy Markdown
Collaborator

Hi, how this PR is going? I would be happy to contribute some changes if you need help. Those symlinks are a bit annoying for me so I would love to get rid of them soon :)

@stachu86

Copy link
Copy Markdown
Contributor Author

I am not sure how to tackle the third point from @Gastron answer. If I'm not mistaken to solve the problem of unwanted symlinks, we could simply do

    elif pathlib.Path(source).is_dir():
        destination = pathlib.Path(source) / filename

in fetch and also we could add savedir to the interface with default value . (for backward compatibility). Other changes mentioned could be then a part of some greater refactoring.

@anautsch
anautsch self-requested a review April 5, 2022 09:07
@anautsch
anautsch changed the base branch from develop to develop-v2 June 1, 2022 15:41
@Gastron Gastron closed this Jan 5, 2023
@Gastron Gastron reopened this Jan 5, 2023
@Adel-Moumen

Copy link
Copy Markdown
Collaborator

Hello,

Any news with this PR please?

Thanks.

@Adel-Moumen

Copy link
Copy Markdown
Collaborator

Dear @stachu86,

Thank you for your contribution to our project through this pull request. Unfortunately, we have decided to close this pull request due to its staleness.

Since the initial submission, our project has undergone significant updates, and we've released several new versions. As a result, we kindly request you to create a new pull request with the latest commits to ensure compatibility and integration with the current codebase.

Please sync your fork with the latest changes from the main repository and address any potential conflicts that may arise during this process. We appreciate your understanding and continued interest in contributing to our project.

If you have any questions or encounter difficulties while creating the new pull request, feel free to reach out. We value your contributions and look forward to seeing an updated pull request from you.

Best regards,
Adel Moumen

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.

6 participants