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

Add seed_everything into CLI side #2836

Merged
merged 5 commits into from
Jan 25, 2024
Merged

Add seed_everything into CLI side #2836

merged 5 commits into from
Jan 25, 2024

Conversation

harimkang
Copy link
Contributor

@harimkang harimkang commented Jan 25, 2024

Summary

@jaegukhyun reported an issue that the seed was not being frozen, so I looked into it.
The seed is frozen after the OTXModel is created, so in some cases the seed was not being frozen.
OTXModel random feature: classification layer initialization

add fix seed function into the CLI.

How to test

seed=0, deterministic=True, model: atss_mobilenetv2
Trial 1: val/map_50: 0.750 val/map_75: 0.502
Trial 2: val/map_50: 0.750 val/map_75: 0.502
Trial 3: val/map_50: 0.750 val/map_75: 0.502

Checklist

  • I have added unit tests to cover my changes.​
  • I have added integration tests to cover my changes.​
  • I have added e2e tests for validation.
  • I have added the description of my changes into CHANGELOG in my target branch (e.g., CHANGELOG in develop).​
  • I have updated the documentation in my target branch accordingly (e.g., documentation in develop).
  • I have linked related issues.

License

  • I submit my code changes under the same Apache License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below).
# Copyright (C) 2023 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

@github-actions github-actions bot added the TEST Any changes in tests label Jan 25, 2024
Copy link
Contributor

@vinnamkim vinnamkim left a comment

Choose a reason for hiding this comment

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

This will disable seed settings on the Python API side. Please find a way to maintain it rather than removing it.

@harimkang
Copy link
Contributor Author

This will disable seed settings on the Python API side. Please find a way to maintain it rather than removing it.

Yes, I think that's a good point.
How do you think we should deliver on the API side?
I think there are three ways.

  1. drive people to use lightning.seed_everything (-> For users who only use OTX, this may be awkward.)
  2. provide the function in 1 as a function of otx like (-> It simply wraps the function that calls seed_everything once more in otx.)
  3. take it as an engine's creation argument (in this case, if the user is already build a module, the seed might not be fixed in some cases).

I don't really have a preference and would love to hear your thoughts.

@vinnamkim
Copy link
Contributor

This will disable seed settings on the Python API side. Please find a way to maintain it rather than removing it.

Yes, I think that's a good point. How do you think we should deliver on the API side? I think there are three ways.

  1. drive people to use lightning.seed_everything (-> For users who only use OTX, this may be awkward.)
  2. provide the function in 1 as a function of otx like (-> It simply wraps the function that calls seed_everything once more in otx.)
  3. take it as an engine's creation argument (in this case, if the user is already build a module, the seed might not be fixed in some cases).

I don't really have a preference and would love to hear your thoughts.

I don't see any reason why the original code cannot setup the seed correctly. Is it because the datamodule already spawned the data pipeline workers before the engine creation?

@harimkang
Copy link
Contributor Author

This will disable seed settings on the Python API side. Please find a way to maintain it rather than removing it.

Yes, I think that's a good point. How do you think we should deliver on the API side? I think there are three ways.

  1. drive people to use lightning.seed_everything (-> For users who only use OTX, this may be awkward.)
  2. provide the function in 1 as a function of otx like (-> It simply wraps the function that calls seed_everything once more in otx.)
  3. take it as an engine's creation argument (in this case, if the user is already build a module, the seed might not be fixed in some cases).

I don't really have a preference and would love to hear your thoughts.

I don't see any reason why the original code cannot setup the seed correctly. Is it because the datamodule already spawned the data pipeline workers before the engine creation?

Yes, that's right.

@harimkang harimkang changed the title Move seed into CLI side Add seed into CLI side Jan 25, 2024
@harimkang harimkang changed the title Add seed into CLI side Add seed_everything into CLI side Jan 25, 2024
@harimkang harimkang marked this pull request as ready for review January 25, 2024 07:33
@harimkang harimkang requested a review from vinnamkim January 25, 2024 07:34
@harimkang harimkang merged commit f914a48 into openvinotoolkit:v2 Jan 25, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TEST Any changes in tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants