You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/08/17 14:14:37 UTC
[GitHub] [tvm] huanmei9 opened a new pull request #8770: [contrib] Fix "custom_path" already exist bug
huanmei9 opened a new pull request #8770:
URL: https://github.com/apache/tvm/pull/8770
fix error when use custom_path parameter in tempdir funciton.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #8770: [contrib] Fix "custom_path" already exist bug
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #8770:
URL: https://github.com/apache/tvm/pull/8770#issuecomment-900699343
Hmm I agree with @tkonolige. It is slightly better to use the existing API provided by python. Please make changes accordingly and then we get the PR merged :-) Thanks a lot!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] huanmei9 edited a comment on pull request #8770: [contrib] Fix "custom_path" already exist bug
Posted by GitBox <gi...@apache.org>.
huanmei9 edited a comment on pull request #8770:
URL: https://github.com/apache/tvm/pull/8770#issuecomment-900758591
Hi @tkonolige @junrushao1994 .
Thanks for your suggestions, I have changed the code according to your suggestions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] leandron commented on a change in pull request #8770: [contrib] Fix "custom_path" already exist bug
Posted by GitBox <gi...@apache.org>.
leandron commented on a change in pull request #8770:
URL: https://github.com/apache/tvm/pull/8770#discussion_r691020815
##########
File path: python/tvm/contrib/utils.py
##########
@@ -99,7 +99,8 @@ def __init__(self, custom_path=None):
self._created_with_keep_for_debug = self._KEEP_FOR_DEBUG
if custom_path:
- os.mkdir(custom_path)
+ if not os.path.exists(custom_path):
+ os.makedirs(custom_path)
Review comment:
Hi @huanmei9, thanks for the PR. Do you mind clarifying on what is the use case for this change? As @Mousius commented, it is not usually good practice to run tests on a polluted directory, that contains files from previous executions.
Apart from that, as others suggested, here a built-in suggestion ([ref](https://docs.python.org/3/library/os.html#os.makedirs)) that can avoid the need to the `if ...` block on this PR:
```suggestion
os.makedirs(custom_path, exist_ok=True)
```
But still I'm not sure we want to be blindly reusing directories as temporary directories for tests. Interested to know some other opinions. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #8770: [contrib] Fix "custom_path" already exist bug
Posted by GitBox <gi...@apache.org>.
Mousius commented on pull request #8770:
URL: https://github.com/apache/tvm/pull/8770#issuecomment-900424765
Hi @huanmei9, just curious what the use case is here? Doesn't this allow for race conditions and pollution by re-using temporary directories?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #8770: [contrib] Fix "custom_path" already exist bug
Posted by GitBox <gi...@apache.org>.
Mousius commented on pull request #8770:
URL: https://github.com/apache/tvm/pull/8770#issuecomment-902557489
Hi @huanmei9,
I would suggest that the current implementation of `custom_path` is robust, consider a typical task which does the following:
1. Creates a temporary directory
2. Writes some files
3. Checks those files
With the current behaviour, if two tasks attempt to do this in parallel (such as parallel tests from pytest-xdist), you get the following:
1. Task A creates a temporary directory
2. Task A writes some files
3. Task B attempts to create a temporary directory - raises Exception that temporary directory already exists
4. Task A checks those files
Task B has failed as it tries to pollute the existing temporary directory.
With this change, you can instead have:
1. Task A creates a temporary directory
2. Task A writes some files
3. Task B attempts to create a temporary directory - no error raised
4. Task B writes some files
5. Task A checks those files - raises assertion error
6. Task B checks those files
Task A has now failed because of the pollution of the temporary directory and the error is also incorrectly attributed to Task A rather than Task B. This scenario only occurs intermittently and is therefore harder to debug.
Therefore, I would suggest that the current implementation of temp dir helps to prevent such concurrency bugs and raises the correct error to ensure we use a new fresh temp directory per invocation - what are your thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] junrushao1994 commented on pull request #8770: [contrib] Fix "custom_path" already exist bug
Posted by GitBox <gi...@apache.org>.
junrushao1994 commented on pull request #8770:
URL: https://github.com/apache/tvm/pull/8770#issuecomment-900699343
Hmm I agree with @tkonolige. It is slightly better to use the existing API provided by python. Please make changes accordingly and then we get the PR merged :-) Thanks a lot!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] leandron commented on a change in pull request #8770: [contrib] Fix "custom_path" already exist bug
Posted by GitBox <gi...@apache.org>.
leandron commented on a change in pull request #8770:
URL: https://github.com/apache/tvm/pull/8770#discussion_r691020815
##########
File path: python/tvm/contrib/utils.py
##########
@@ -99,7 +99,8 @@ def __init__(self, custom_path=None):
self._created_with_keep_for_debug = self._KEEP_FOR_DEBUG
if custom_path:
- os.mkdir(custom_path)
+ if not os.path.exists(custom_path):
+ os.makedirs(custom_path)
Review comment:
Hi @huanmei9, thanks for the PR. Do you mind clarifying on what is the use case for this change? As @Mousius commented, it is not usually good practice to run tests on a polluted directory, that contains files from previous executions.
Apart from that, as others suggested, here a built-in suggestion ([ref](https://docs.python.org/3/library/os.html#os.makedirs)) that can avoid the need to the `if ...` block on this PR:
```suggestion
os.makedirs(custom_path, exist_ok=True)
```
But still I'm not sure we want to be blindly reusing directories as temporary directories for tests. Interested to know some other opinions. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] huanmei9 closed pull request #8770: [contrib] Fix "custom_path" already exist bug
Posted by GitBox <gi...@apache.org>.
huanmei9 closed pull request #8770:
URL: https://github.com/apache/tvm/pull/8770
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on pull request #8770: [contrib] Fix "custom_path" already exist bug
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #8770:
URL: https://github.com/apache/tvm/pull/8770#issuecomment-900478536
I believe the correct way to do this is [`os.makedirs`](https://docs.python.org/3/library/os.html#os.makedirs) with `exist_ok=True`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on pull request #8770: [contrib] Fix "custom_path" already exist bug
Posted by GitBox <gi...@apache.org>.
Mousius commented on pull request #8770:
URL: https://github.com/apache/tvm/pull/8770#issuecomment-900424765
Hi @huanmei9, just curious what the use case is here? Doesn't this allow for race conditions and pollution by re-using temporary directories?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] huanmei9 commented on pull request #8770: [contrib] Fix "custom_path" already exist bug
Posted by GitBox <gi...@apache.org>.
huanmei9 commented on pull request #8770:
URL: https://github.com/apache/tvm/pull/8770#issuecomment-903801393
Hi, @Mousius .
The situation you describe is indeed possible, Thanks for your suggestion.
(●'◡'●)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] huanmei9 edited a comment on pull request #8770: [contrib] Fix "custom_path" already exist bug
Posted by GitBox <gi...@apache.org>.
huanmei9 edited a comment on pull request #8770:
URL: https://github.com/apache/tvm/pull/8770#issuecomment-900758591
Hi @tkonolige @junrushao1994 .
Thanks for your suggestions, I have changed the code according to your suggestions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tkonolige commented on pull request #8770: [contrib] Fix "custom_path" already exist bug
Posted by GitBox <gi...@apache.org>.
tkonolige commented on pull request #8770:
URL: https://github.com/apache/tvm/pull/8770#issuecomment-900478536
I believe the correct way to do this is [`os.makedirs`](https://docs.python.org/3/library/os.html#os.makedirs) with `exist_ok=True`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] huanmei9 commented on pull request #8770: [contrib] Fix "custom_path" already exist bug
Posted by GitBox <gi...@apache.org>.
huanmei9 commented on pull request #8770:
URL: https://github.com/apache/tvm/pull/8770#issuecomment-900758591
Hi @tkonolige @junrushao1994 .
Thanks for your suggestions, I have changed the code according to your suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] huanmei9 commented on pull request #8770: [contrib] Fix "custom_path" already exist bug
Posted by GitBox <gi...@apache.org>.
huanmei9 commented on pull request #8770:
URL: https://github.com/apache/tvm/pull/8770#issuecomment-902361519
Hi, @leandron .
Sorry for the late reply.
My use case is to check the build fils generated by zephyr in MicroTVM, such as .map and .hex files.
In addition, regardless of the use case, the tempdir function provides a custom_path interface, so it should be robust enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] huanmei9 commented on pull request #8770: [contrib] Fix "custom_path" already exist bug
Posted by GitBox <gi...@apache.org>.
huanmei9 commented on pull request #8770:
URL: https://github.com/apache/tvm/pull/8770#issuecomment-900758591
Hi @tkonolige @junrushao1994 .
Thanks for your suggestions, I have changed the code according to your suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org