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

Onnx Feature Scorer #452

Merged
merged 12 commits into from
Sep 21, 2023
Merged

Onnx Feature Scorer #452

merged 12 commits into from
Sep 21, 2023

Conversation

Marvin84
Copy link
Contributor

@Marvin84 Marvin84 commented Sep 21, 2023

Starting with the apptek version, I renamed some variables in order to decouple this from the old GMM feature scorer. Moreover, 2 internal rasr variables with their default values are added.

The outdated wrong FH label scorer should be deleted.

…ed later on with the current version and once integrated in master branch
@Atticus1806
Copy link
Contributor

Atticus1806 commented Sep 21, 2023

I thought this PR was about adding a new feature scorer not replacing and old?

Also I think rasr is not defined.

@christophmluscher
Copy link
Contributor

christophmluscher commented Sep 21, 2023

I agree with @Atticus1806 we should add a Featurescorer not replace

Or is the tf label scorer removed from RASR code?

@christophmluscher
Copy link
Contributor

I would add onnx feature scorer and later refactor the FH label scorer

@Marvin84
Copy link
Contributor Author

I thought this PR was about adding a new feature scorer not replacing and old?

I would add onnx feature scorer and later refactor the FH label scorer

We discussed this some time ago and I wondered why this FS is still there. The code that is here would not work in any setup or branch of rasr. First of all, this is an outdated FH FS and has ended up in this module only by error. Secondly, the merging of the 24bit now we can have FH in the master of rasr however, there is still no feature scorer in the master. Therefore, this code should be deleted.

@Marvin84
Copy link
Contributor Author

I agree with @Atticus1806 we should add a Featurescorer not replace

Or is the tf label scorer removed from RASR code?

It has never been in the master version. This is from 4 years ago. Refactoring it would not bring anything since the new FS is completely different

rasr/feature_scorer.py Outdated Show resolved Hide resolved
Copy link
Contributor

@JackTemaki JackTemaki left a comment

Choose a reason for hiding this comment

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

some small things

rasr/feature_scorer.py Outdated Show resolved Hide resolved
rasr/feature_scorer.py Show resolved Hide resolved
rasr/feature_scorer.py Show resolved Hide resolved
Copy link
Contributor

@Atticus1806 Atticus1806 left a comment

Choose a reason for hiding this comment

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

If its deprecated and all tests pass I am not against removing, I don't know the details I just noticed that this was different from what I expected :).

Just make sure with AppTek I guess (do their tests claim completeness? If so I guess its fine if tests pass)

Once that is clear and the typing is changeed I think it can be approved :)

rasr/feature_scorer.py Outdated Show resolved Hide resolved
rasr/feature_scorer.py Outdated Show resolved Hide resolved
rasr/feature_scorer.py Show resolved Hide resolved
rasr/feature_scorer.py Outdated Show resolved Hide resolved
rasr/feature_scorer.py Outdated Show resolved Hide resolved
rasr/feature_scorer.py Outdated Show resolved Hide resolved
rasr/feature_scorer.py Outdated Show resolved Hide resolved
rasr/feature_scorer.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Atticus1806 Atticus1806 left a comment

Choose a reason for hiding this comment

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

Thanks :)

@Atticus1806 Atticus1806 self-requested a review September 21, 2023 14:11
rasr/feature_scorer.py Outdated Show resolved Hide resolved
rasr/feature_scorer.py Outdated Show resolved Hide resolved
rasr/feature_scorer.py Outdated Show resolved Hide resolved
rasr/feature_scorer.py Outdated Show resolved Hide resolved
rasr/feature_scorer.py Outdated Show resolved Hide resolved
rasr/feature_scorer.py Outdated Show resolved Hide resolved
@curufinwe
Copy link
Contributor

Just make sure with AppTek I guess (do their tests claim completeness? If so I guess its fine if tests pass)

We never used the deleted scorer, so it should be fine.
The tests we have cover almost everything that is related to the training / running of our production systems so if they are green you should be ok.

@curufinwe
Copy link
Contributor

PS: the current failing check is because of this:

  File "test/hash_test/check_hashes.py", line 17, in <module>
    config_manager.load_configs([args.function])
  File "/opt/atlassian/pipelines/agent/build/.nox/check_hashes-3-8/lib/python3.8/site-packages/sisyphus/loader.py", line 114, in load_configs
    self.load_config_file(filename)
  File "/opt/atlassian/pipelines/agent/build/.nox/check_hashes-3-8/lib/python3.8/site-packages/sisyphus/loader.py", line 54, in load_config_file
    config = importlib.import_module(module_name)
  File "/usr/local/lib/python3.8/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1014, in _gcd_import
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 671, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 843, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/opt/atlassian/pipelines/agent/build/config/create_corpora.py", line 2, in <module>
    from apptek_asr.data.common import BuildCorpusFromCSV, BuildCorpusFromBliss
  File "/opt/atlassian/pipelines/agent/build/apptek_asr/data/common/__init__.py", line 1, in <module>
    from .build_corpus import *
  File "/opt/atlassian/pipelines/agent/build/apptek_asr/data/common/build_corpus.py", line 27, in <module>
    from i6_core.corpus import (
  File "/opt/atlassian/pipelines/agent/build/i6_core/corpus/__init__.py", line 2, in <module>
    from .costa import *
  File "/opt/atlassian/pipelines/agent/build/i6_core/corpus/costa.py", line 5, in <module>
    from i6_core.features.common import raw_audio_flow
  File "/opt/atlassian/pipelines/agent/build/i6_core/features/__init__.py", line 1, in <module>
    from .common import *
  File "/opt/atlassian/pipelines/agent/build/i6_core/features/common.py", line 18, in <module>
    import i6_core.rasr as rasr
  File "/opt/atlassian/pipelines/agent/build/i6_core/rasr/__init__.py", line 3, in <module>
    from .feature_scorer import *
  File "/opt/atlassian/pipelines/agent/build/i6_core/rasr/feature_scorer.py", line 19, in <module>
    from typing import Union, Dict, Bool
ImportError: cannot import name 'Bool' from 'typing' (/usr/local/lib/python3.8/typing.py)

@Marvin84
Copy link
Contributor Author

PS: the current failing check is because of this:

Yes, solved

rasr/feature_scorer.py Outdated Show resolved Hide resolved
@Marvin84
Copy link
Contributor Author

@curufinwe I give you all passed tests and 3 approvals, do I get a merge from you? :)

Copy link
Contributor

@NeoLegends NeoLegends left a comment

Choose a reason for hiding this comment

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

Nice

@JackTemaki JackTemaki merged commit d8b6fa6 into main Sep 21, 2023
4 checks passed
@JackTemaki JackTemaki deleted the onnx-fs branch September 27, 2023 09:06
Atticus1806 pushed a commit that referenced this pull request Oct 2, 2023
* added onnx FS and deleted the old FH label scorer, latter will be added later on with the current version and once integrated in master branch
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.

8 participants