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

Bug/sg 000 merge failure for datasetparams #1140

Merged
merged 11 commits into from
Jun 7, 2023

Conversation

shaydeci
Copy link
Collaborator

@shaydeci shaydeci commented Jun 6, 2023

Recent changes in #1092 caused pose estimation recipe tests to crash https://app.circleci.com/pipelines/github/Deci-AI/super-gradients/7748/workflows/5d210255-4bed-4736-b4d4-b693d6bc91e1/jobs/23382

This is becaue there are overrides for dataset params from the command line, causing the merge to fail.

Before calling merge we neeed to set the two configs's struct to false, other wise if one includes an attribute which the other one does not have - the merge will fail.

@shaydeci shaydeci marked this pull request as ready for review June 6, 2023 12:20
BloodAxe
BloodAxe previously approved these changes Jun 6, 2023
Copy link
Collaborator

@BloodAxe BloodAxe left a comment

Choose a reason for hiding this comment

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

I'm wondering into how many unexpected things we will run with this config merging, but..
LGTM )

Louis-Dupont
Louis-Dupont previously approved these changes Jun 6, 2023
Copy link
Contributor

@Louis-Dupont Louis-Dupont left a comment

Choose a reason for hiding this comment

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

LGTM

ofrimasad
ofrimasad previously approved these changes Jun 6, 2023
Copy link
Collaborator

@ofrimasad ofrimasad left a comment

Choose a reason for hiding this comment

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

LGTM

@shaydeci shaydeci dismissed stale reviews from ofrimasad, Louis-Dupont, and BloodAxe via a451d97 June 6, 2023 12:35
…ilure_for_datasetparams

# Conflicts:
#	src/super_gradients/training/utils/checkpoint_utils.py
@BloodAxe
Copy link
Collaborator

BloodAxe commented Jun 6, 2023

Pls add test to tests/unit_tests/detection_dataset_test.py ^_^

@shaydeci
Copy link
Collaborator Author

shaydeci commented Jun 7, 2023

Pls add test to tests/unit_tests/detection_dataset_test.py ^_^

Added a simple test that uses dataloaders.get with dataset_params that indclude an entry which is not present in the yaml file (this is what caused the crash).

Copy link
Collaborator

@BloodAxe BloodAxe left a comment

Choose a reason for hiding this comment

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

LGTM

@shaydeci shaydeci merged commit 83ce129 into master Jun 7, 2023
1 check passed
@shaydeci shaydeci deleted the bug/SG-000_merge_failure_for_datasetparams branch June 7, 2023 08:26
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.

None yet

4 participants