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

[OTX][HOT-FIX] Revert otx cli tools with data.yaml check #1530

Merged
merged 1 commit into from
Jan 18, 2023

Conversation

harimkang
Copy link
Contributor

@harimkang harimkang commented Jan 16, 2023

Summary

Due to the effect of that #1497, data.yaml of otx-workspace becomes unusable, so I revert this.

@harimkang harimkang requested a review from a team as a code owner January 16, 2023 11:45
@github-actions github-actions bot added the CLI Any changes in OTE CLI label Jan 16, 2023
@cih9088
Copy link
Contributor

cih9088 commented Jan 16, 2023

The problem of original implementation was that ./data.yaml used while testing sometimes does not get removed.
If ./data.yaml is not removed, all the following tests would be failed. I didn't look into the root cause though.
Revert it for now and if a similar issue comes up, lets fix them then.

@harimkang
Copy link
Contributor Author

The problem of original implementation was that ./data.yaml used while testing sometimes does not get removed. If ./data.yaml is not removed, all the following tests would be failed. I didn't look into the root cause though. Revert it for now and if a similar issue comes up, lets fix them then.

Oh, there was such an issue. Let me take a look.

@sungmanc
Copy link
Contributor

@harimkang @cih9088 , Did you execute the parallel tests ? (conduct >= 2 classification/detection/segmentation tests at the same time).

When I executed 2 task tests, I got the same error as @cih9088 's said.

@harimkang
Copy link
Contributor Author

@harimkang @cih9088 , Did you execute the parallel tests ? (conduct >= 2 classification/detection/segmentation tests at the same time).

When I executed 2 task tests, I got the same error as @cih9088 's said.

Hi @sungchul2, I think the issue is caused by the function used in the test below. Could you please confirm?

@cih9088
Copy link
Contributor

cih9088 commented Jan 17, 2023

When I executed 2 task tests, I got the same error as @cih9088 's said.

Yes, exactly. As I said, if you have .data.yaml in your CWD, because of whatever reasons, like pytest didn't remove it properly or one executed tests parallel, which means at some point you got data.yaml in your CWD, you would have an error.

@harimkang
Copy link
Contributor Author

When I executed 2 task tests, I got the same error as @cih9088 's said.

Yes, exactly. As I said, if you have .data.yaml in your CWD, because of whatever reasons, like pytest didn't remove it properly or one executed tests parallel, which means at some point you got data.yaml in your CWD, you would have an error.

As sungman said, if we run multiple tests at the same time, it seems to be reproduced. I think maybe the cause I found is correct. I guess this needs to be addressed as I don't think test should affect root.

@sungmanc
Copy link
Contributor

sungmanc commented Jan 17, 2023

When I executed 2 task tests, I got the same error as @cih9088 's said.

Yes, exactly. As I said, if you have .data.yaml in your CWD, because of whatever reasons, like pytest didn't remove it properly or one executed tests parallel, which means at some point you got data.yaml in your CWD, you would have an error.

Let's assume this case,

test process 1: classification test
test process 2: detection test

Then, test process 1 made data.yaml during the self-sl test. In the mean time, test process 2 might get error because there was data.yaml file made by test process 1.

So, if we don't use parallel tests, then it will be fine

@sungmanc
Copy link
Contributor

sungmanc commented Jan 17, 2023

When I executed 2 task tests, I got the same error as @cih9088 's said.

Yes, exactly. As I said, if you have .data.yaml in your CWD, because of whatever reasons, like pytest didn't remove it properly or one executed tests parallel, which means at some point you got data.yaml in your CWD, you would have an error.

As sungman said, if we run multiple tests at the same time, it seems to be reproduced. I think maybe the cause I found is correct. I guess this needs to be addressed as I don't think test should affect root.

Agreed. and this problem will be resolved after the merge of Datumaro PR. @sungchul2 , @cih9088 , @harimkang .
So, how about keep this as is?

Because, when we use the Datumaro, we don't need to directly make data.yaml for test case.

Making .yaml file will be removed
image

@sungchul2
Copy link
Contributor

sungchul2 commented Jan 17, 2023

I agree. After Datumaro integration, I think we can remove the function.

@harimkang harimkang merged commit b1a8c03 into feature/otx Jan 18, 2023
@harimkang harimkang deleted the hot-fix-workspace branch January 18, 2023 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI Any changes in OTE CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants