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

Removing of audio fetching from inference interface #1268

Closed

Conversation

stachu86
Copy link
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
Copy link
Collaborator

Gastron commented Jan 24, 2022

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

@stachu86
Copy link
Contributor Author

stachu86 commented Jan 24, 2022

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

@Gastron
Copy link
Collaborator

Gastron commented Jan 24, 2022

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
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
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 added this to In progress in CI/CD via automation Apr 21, 2022
@anautsch anautsch moved this from In progress to Performance & housekeeping in CI/CD Apr 21, 2022
@anautsch anautsch changed the base branch from develop to develop-v2 June 1, 2022 15:41
@Gastron Gastron closed this Jan 5, 2023
CI/CD automation moved this from Performance & housekeeping to Closed Jan 5, 2023
@Gastron Gastron reopened this Jan 5, 2023
CI/CD automation moved this from Closed to In progress Jan 5, 2023
@Adel-Moumen
Copy link
Collaborator

Hello,

Any news with this PR please?

Thanks.

@Adel-Moumen
Copy link
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

CI/CD automation moved this from In progress to Closed Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
CI/CD
Closed
Development

Successfully merging this pull request may close these issues.

None yet

5 participants