plex_run adds "python" annotation by default#600
Conversation
… added in dev/example.py to confirm
LAB-512 default add 'python' annotation for bacalhau submissions made through pip pkg
This enables better insights into how many users interact through our notebooks. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@hevans66 does this change anything related to how we make our metabase queries? |
| cmd.append(f"--annotations={annotations.join(',')}") | ||
| # Ensure "python" is always in the annotations list | ||
| if "python" not in annotations: | ||
| annotations.append("python") |
There was a problem hiding this comment.
Its arguable whether your intended effect is to mutate whatever list was passed in. Since I feel like this "python" is an internal annotation I would change this to annotations = annotations + ["python"]. That way we don't mutate the list of annotations a user gave us. This a very nitpick thing though, feel free to leave this.
python/src/plex/__init__.py
Outdated
| if "python" not in annotations: | ||
| annotations.append("python") | ||
|
|
||
| cmd.append(f"--annotations={','.join(annotations)}") |
There was a problem hiding this comment.
Staring at this more closely. I don't think it woks. Its causing one big string with commas in it, instead of a list of strings.
I believe there is nothing in the cobra go package that would separate these out into individual strings from the annotations comma separated list. Instead we need to pass the same flag multiple times to have it interpret it as an array.
I think this needs to look something like
cmd.extend([f"-a={annotation}" for annotation in annotations])There was a problem hiding this comment.
Thanks for flagging @hevans66. This change should fix it so we get a new string for each annotation. Each string makes up an individual element in the golang array.
plex/python/src/plex/__init__.py
Lines 180 to 184 in 2362adb
|
Vercel check can be ignored |
The
plex_runcommand now automatically includes a "python" annotation by default. This is to distinguish between using plex in Python and Go for analytics purposes.To Test:
To Do Before Merging:
[ ] Upgrade pip pkg version number in python/setup.py
[ ] Publish new pip pkg version