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
Misc adjustments to dependency tracking #1389
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1389 +/- ##
==========================================
- Coverage 79.53% 73.17% -6.37%
==========================================
Files 279 279
Lines 13849 13562 -287
==========================================
- Hits 11015 9924 -1091
- Misses 2834 3638 +804
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
@@ -4,10 +4,10 @@ | |||
"FileDependenciesStrategy", | |||
] | |||
|
|||
import logging |
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.
We have our custom logger somewhere. It has some settings to simplify filtering etc. I think it's:
from neptune.internal.utils.logger import logger
@@ -32,7 +32,9 @@ def log_dependencies(self, run: "Run") -> None: | |||
class InferDependenciesStrategy(DependencyTrackingStrategy): | |||
def log_dependencies(self, run: "Run") -> None: | |||
try: | |||
dependencies_str = subprocess.check_output([sys.executable, "-m", "pip", "freeze"]).decode("utf-8") | |||
dependencies_str = subprocess.check_output([sys.executable, "-m", "pip", "list", "--format=freeze"]).decode( |
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.
Have you tried the importlib
approach? I've been suggesting something like:
import sys
if sys.version_info >= (3, 8):
from importlib.metadata import distributions
else:
from importlib_metadata import distributions
def list_installed_packages():
dists = list(sorted(distributions(), key=lambda d: d.metadata['Name']))
for dist in dists:
name, version = dist.metadata['Name'], dist.metadata['Version']
print(f'{name}=={version}')
def main():
list_installed_packages()
if __name__ == '__main__':
main()
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.
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.
Just tried. This seems to work very well!
Lets do this instead of subprocess
log and error instead of warning if path does not exist upload as file, not file set remove warning test case adjust unit tests test if error was logged with non existent file adjust units update changelog update changelog use neptune logger instead of logging.error use importlib instead of subprocess
c382900
to
ae5a6ce
Compare
|
||
### Changes | ||
- Dependency tracking feature will log an error if a given file path doesn't exist ([#1389](https://github.com/neptune-ai/neptune-client/pull/1389)) | ||
- Use `pip list --format=freeze` instead of `pip freeze` in dependency tracking ([#1389](https://github.com/neptune-ai/neptune-client/pull/1389)) |
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.
Not sure if it was fixed, but this note is misleading as we're not using it here.
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.
@twolodzko @SiddhantSadangi I'm addressing this by the way in this PR: #1391
Before submitting checklist