-
Notifications
You must be signed in to change notification settings - Fork 67
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
[SYNPY-1320] Upload benchmark + Documentation #1012
Conversation
Hello @BryanFauble! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2023-11-16 17:56:42 UTC |
newObj = self.restGET(obj.getByNameURI(obj.name)) | ||
newObj.update(obj) | ||
obj = type(obj)(**newObj) | ||
with tracer.start_as_current_span( |
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.
Adding this with
is the only real change here - The sync functions multi-thread at this point and OTEL context was not propagating
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.
Do you mind adding the explanation of opentelemetry_context parameter in the function docstring?
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.
🔥 Looks great to me! I'm going to pre-approve.
I do realize we've baked in otel contexts throughout the code which adds a layer of nesting sometimes which is ok. In the beginning, I was wondering if there was a way for that code to exist without the opentel packages installed (so we don't always require the dependency) but this is great.
Yeah I agree with you here - Unfortunately the way we are doing manually instrumenting the client + the way multi-threading is set up leading to some less than savory code. Fortunately with all testing I've done with asyncio - This kind of context propagation is no longer required. |
README.md
Outdated
``` | ||
|
||
#### Sync a local directory to synapse | ||
This is the recommended way of synchronizing more than one file or directory to a synapse project is through the use of `synapseutils`. Using this library let's us handle scheduling everything required to sync an entire directory tree. Read more about the manifest file format in [`synapseutils.syncToSynapse`](https://python-docs.synapse.org/build/html/articles/synapseutils.html#synapseutils.sync.syncToSynapse) |
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.
Nit: could you rephrase this sentence? It has two verbs.
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 modified this slightly - Let me know if this new version sounds better:
This is the recommended way of synchronizing more than one file or directory to a synapse project through the use of
synapseutils
. Using this library allows us to handle scheduling everything required to sync an entire directory tree. Read more about the manifest file format insynapseutils.syncToSynapse
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.
Looks great!
Thanks for working on this. I approved the PR. |
Problem:
Solution:
syn.store
functionTesting: