-
Notifications
You must be signed in to change notification settings - Fork 14
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
plex_run
adds "python" annotation by default
#600
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
# Add each annotation as a separate parameter to cmd | |
for annotation in annotations: | |
cmd.append(f"--annotations={annotation}") | |
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.
🚢 🇮🇹
Vercel check can be ignored |
The
plex_run
command 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