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

move mmdet templates #985

Merged
merged 18 commits into from
Mar 19, 2022
Merged

move mmdet templates #985

merged 18 commits into from
Mar 19, 2022

Conversation

Ilya-Krylov
Copy link
Contributor

No description provided.

@Ilya-Krylov Ilya-Krylov requested a review from a team March 18, 2022 12:04
@Ilya-Krylov
Copy link
Contributor Author

run ote_sdk tests

@github-actions github-actions bot added CLI Any changes in OTE CLI DEPENDENCY Any changes in any dependencies (new dep or its version) should be produced via Change Request on PM ALGO Any changes in OTX Algo Tasks implementation TEST Any changes in tests labels Mar 18, 2022
@Ilya-Krylov
Copy link
Contributor Author

ote-test/1451/

@Ilya-Krylov
Copy link
Contributor Author

ote-test/1452/

@Ilya-Krylov
Copy link
Contributor Author

run ote_sdk tests

@Ilya-Krylov
Copy link
Contributor Author

ote-test/1453/

@aiarkinx
Copy link

run ote-test

@@ -31,6 +31,7 @@ def parse_args():
"--root", help="A root dir where templates should be searched.", default="."
)
parser.add_argument("--task_type")
parser.add_argument("--experimental", action="store_true")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe turn it on by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe, but in this case I would set it to False while checking ote_cli features like train, eval, export and etc. Otherwise testing will take much more longer. However experimental templates have to be tested as well, I don't know how it can be done during reasonable time.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is testing code, this is a user-facing API. And IMO having this feature disabled here means that we don't want users to use "experimental" templates in OTE. So, I think about switching the default.

This being said, I agree that non-experimental templates should be tested in the first place, and experimental ones may be skipped to decrease CI load.

@@ -0,0 +1,42 @@
# Copyright (C) 2021 Intel Corporation
#
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as a discussion point.

  1. As we now have common set of tasks/utils/whatever for all mmdetection-powered templates, it looks safe to remove extra level of nesting (detection) here. But probably not in this PR.
  2. Having mmdetection directory containing detection_tasks makes the story a bit complicated. Maybe we should not reference any particular algo backend in the folders' tree of OTE at all and leave it being hidden under the neutral submodule? I foresee difficulties if we end up having multiple detection backends though. Any thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. makes sense
  2. not sure that I fully understand, but "external" and "mmdetection" (and other) names are kept here because of CI. I made this transition so that all paths such as "external/mmdetection/init_venv.sh", "external/mmdetection/tests", etc are not changed. I guess these paths are hardcoded, @LeonidBeynenson , right? Or are you, @druzhkov-paul , telling about different things?

Copy link
Contributor

Choose a reason for hiding this comment

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

You captured my point right.

I see, that we have limitation at the CI side. @LeonidBeynenson could you please estimate how difficult and time-consuming would that be to support modified directories structure? I mean 100% transition, not "almost completed" state.

At the same time do we have an agreement that such renaming is a reasonable thing to do? Or it does not pay off taking into account all of the required changes in the infrastructure?

druzhkov-paul
druzhkov-paul previously approved these changes Mar 18, 2022
@Ilya-Krylov
Copy link
Contributor Author

run ote_sdk tests

@Ilya-Krylov
Copy link
Contributor Author

run ote_sdk tests

@Ilya-Krylov
Copy link
Contributor Author

ote-test/1468/ 🟡

@Ilya-Krylov Ilya-Krylov merged commit 5931ec7 into develop Mar 19, 2022
@Ilya-Krylov Ilya-Krylov deleted the ik/move_mmdet_templates branch March 19, 2022 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ALGO Any changes in OTX Algo Tasks implementation CLI Any changes in OTE CLI DEPENDENCY Any changes in any dependencies (new dep or its version) should be produced via Change Request on PM TEST Any changes in tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants