add path as an option for uploading attachments #1331
Conversation
| attachments = { | ||
| a: v for a, v in self.attachments.items() if isinstance(v, tuple) | ||
| } |
There was a problem hiding this comment.
This was done strictly for typing
There was a problem hiding this comment.
If so might be worth doing in a for loop and raising an error if not. I think would be unexpected. But mypy probably knows better than me, so there might be a case we're not handling here
There was a problem hiding this comment.
v is never not a tuple here, this is just because of the union types I believe and it thinks that something could be passed here that is never passed
| class Config: | ||
| """Configuration class for the schema.""" | ||
|
|
||
| arbitrary_types_allowed = True | ||
|
|
There was a problem hiding this comment.
Needed to allow the AttachmentInfo
| acc_parts.append( | ||
| ( | ||
| f"attachment.{op.id}.{n}", | ||
| ( | ||
| None, | ||
| open(file_path, "rb"), # type: ignore[arg-type] |
There was a problem hiding this comment.
do we not need to read this?
There was a problem hiding this comment.
I don't think so, I believe the multipart endpoint allows a buffer to be passed (I also added a test that should cover this)
There was a problem hiding this comment.
This is pretty cve worthy so we should be careful about cases where this is allowed/encouraged
There was a problem hiding this comment.
My suggestion would be to require either an environment variable of LANGSMITH_DANGEROUSLY_ALLOW_FILESYSTEM or some client init variable saying this. Otherwise throw an error.
There was a problem hiding this comment.
We need to explicitly close the files the are opened here, using some try, finally pattern. I would structure the code such that the multipart form is assembled and sent within the same method we are opening the file handles:
try:
# Prepare the files for the multipart request
# Make the POST request
finally:
# Explicitly close the files
| @@ -370,7 +368,9 @@ class RunBase(BaseModel): | |||
| tags: Optional[List[str]] = None | |||
| """Tags for categorizing or annotating the run.""" | |||
|
|
|||
| attachments: Attachments = Field(default_factory=dict) | |||
| attachments: Union[Attachments, Dict[str, AttachmentInfo]] = Field( | |||
There was a problem hiding this comment.
is this related to this pr?
There was a problem hiding this comment.
yeah need this for the changes in read_run
There was a problem hiding this comment.
Left comments. I think main issue is the arbitrary file read, which I could be convinced isn't a cve if we feel data exfiltration to langsmith is by design. But it feels moderately easy to construct a payload as a user of an app that would make the server stream /etc/passwd to whichever langsmith url you have, which is unideal.
| acc_parts.append( | ||
| ( | ||
| f"attachment.{op.id}.{n}", | ||
| ( | ||
| None, | ||
| open(file_path, "rb"), # type: ignore[arg-type] |
There was a problem hiding this comment.
We need to explicitly close the files the are opened here, using some try, finally pattern. I would structure the code such that the multipart form is assembled and sent within the same method we are opening the file handles:
try:
# Prepare the files for the multipart request
# Make the POST request
finally:
# Explicitly close the files
| @@ -377,6 +378,41 @@ def test_persist_update_run(langchain_client: Client) -> None: | |||
| langchain_client.delete_project(project_name=project_name) | |||
|
|
|||
|
|
|||
| def test_update_run_attachments(langchain_client: Client) -> None: | |||
There was a problem hiding this comment.
would be good to add a test case for traceable as well
There was a problem hiding this comment.
added one in test_runs because that is where the previous traceable tests were
| try: | ||
| file.close() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
probably good to log something here
No description provided.