-
Notifications
You must be signed in to change notification settings - Fork 89
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
Change LANGCHAIN_REVISION_ID to revision_id; use git describe
as backup; don't overwrite metadata from env variables
#365
Conversation
…ckup; don't overwrite metadata from env variables
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 alright. 1 request and 1 question
python/langsmith/env/_runtime_env.py
Outdated
.strip() | ||
.decode() | ||
) | ||
except Exception: |
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 think we should catch BaseException - include runtime errors and the like
python/langsmith/env/_runtime_env.py
Outdated
|
||
try: | ||
return ( | ||
subprocess.check_output( |
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.
Any reason not to use _exec_git
?
python/langsmith/env/__init__.py
Outdated
@@ -27,4 +27,5 @@ | |||
"get_runtime_environment", | |||
"get_system_metrics", | |||
"get_git_info", | |||
"exec_git", |
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.
Let's not export this on the env
level (don't want to update our public API). We can still use with other internal files though
python/langsmith/env/_git.py
Outdated
return None | ||
except subprocess.CalledProcessError as e: | ||
logger.debug(f"Error running git command: {e}") | ||
except (FileNotFoundError, subprocess.CalledProcessError, BaseException): |
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.
Could just do BaseException at this point! Captures all cases
python/langsmith/env/_runtime_env.py
Outdated
@functools.lru_cache(maxsize=1) | ||
def _get_default_revision_id() -> Optional[str]: | ||
"""Get the default revision ID based on `git describe`.""" | ||
import subprocess |
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.
non longer needed
No description provided.