-
Notifications
You must be signed in to change notification settings - Fork 128
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
Update bug fix issue template #676
Conversation
Remove most of the irrelevant CUDA / PyTorch deps.
Codecov Report
@@ Coverage Diff @@
## development #676 +/- ##
===============================================
- Coverage 88.36% 88.30% -0.07%
===============================================
Files 125 125
Lines 7565 7565
===============================================
- Hits 6685 6680 -5
- Misses 880 885 +5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a straight-forward ship-it considering there are only a few deviations from torch/utils/collect_env.py
. I've left a few comments/questions
# Caffe2 uses a copyright model similar to Caffe: each contributor holds | ||
# copyright over their contributions to Caffe2. The project versioning records | ||
# all such contribution and copyright details. If a contributor wants to further | ||
# mark their specific copyright on a particular contribution, they should | ||
# indicate their copyright solely in the commit message of the change when it is | ||
# committed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the source of these headers, and are they are necessary considering that the torch source file does not have them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm good question, I just inlined the entirety of PyTorch's LICENSE file, since it uses a somewhat non-standard text
if out is None: | ||
return out | ||
# Comment starting at beginning of line | ||
comment_regex = re.compile(r"^#.*\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this cover comments using triple quotes? """ Comment """
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn't, but I think this is just to filter the output of conda list
, which sometimes prefixes comment lines with # <something>
from enum import Enum | ||
from typing import Any, Dict, List, Optional, Tuple, TypeVar, Union |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably move this to another PR since it's an unrelated (but valid) change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good catch, it crept into this PR as a tiny fixup 🙂
Many thanks for the review @dpacgopinath! |
conda
from the list of installation sources (we don't have a conda package)collect_env.py
from PyTorch repo to this repo.collect_env.py
for CompilerGym by removing irrelevant CUDA / PyTorch dependencies.