Skip to content
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

PERF-3268: Add docs about python actors #776

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

brad-devlugt
Copy link
Contributor

No description provided.

Copy link
Contributor

@dalyd dalyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe break this into a formatting PR and a text PR? Deleting all the trailing whitespace is a good thing, but a bit distracting in this PR. I'd be fine just pushing the whitespace change by itself.

docs/using.md Outdated
1. [pipe creation failed (24): Too many open files](#orga7ab911)
2. [Actor integration tests fail locally](#orgb084b49)
3. [The Loader agent requires thread count set on both Actor and phase level](#org97681a9)
- [Table of Contents](#table-of-contents)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you intentionally change from numbered list to bulleted list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my mistake, reverted this

docs/using.md Outdated
@@ -832,6 +835,12 @@ This will create new Actor .cpp and .h files, an example workload yaml, as well

If your configuration wants to use logic, ifs, or anything beyond simple or existing commands in a loop, then consider writing your own Actor. It doesn't need to be super general or even super well-tested or refactored. Genny is open to submissions and you own whatever Actor you write. No need to loop TIPS in to your custom actor's PR unless you'd just like a second look.

## Creating a Python Actor

Python actors are a highly experimental feature that lets you write actor code in python instead of C++. Note that using Python actors isn't recommended at the current time due to the experimental nature of the feature. If you have a use-case where you think that a Python actor could be useful, please reach out to the performance team. See [example_actor.py](../src/cast_python/src/example_actor.py) for an example of how to write a Python actor and [mongosync_actor.py](../src/cast_python/src/mongosync_actor.py) for an example of a Python actor that is currently used in a real performance workload.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could I suggest adding:

Python actors are useful for two cases: 1. Adding some form of test orchestration not currently supported by DSI and 2. For prototyping support for some new feature. For the second case we encourage you to rewrite your Python actor in C++ if the prototype is successful. We particularly discourage the use of the Python actor for cases in which the actor performance itself is important (e.g., measuring anything that is expected to take milliseconds to complete)

I'd strike the "highly" above. You are using it for important tests, so hopefully it's not too experimental. You could consider linking to your use.

Could you wrap this line and the next? They are both very long lines.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to emphasize that this Actor is entirely community-supported. The "owner" of this Actor is product-perf.

It is not part of Genny core, and it relies on undocumented APIs (namely the python venv) that could break without expectation that Genny Authors would fix it.

I would strongly discourage any additional use of this Actor in production workloads until we can formally adopt it into Genny core. I view this Actor as entirely "one instance of trying out a python thing," not "how you might consider writing a new Actor or workload in Genny"

I'm almost inclined to say let's not document this since it might lead someone to incorrect conclusions no matter how strongly we emphasize the lack of support from Genny proper.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd strike the "highly" above. You are using it for important tests, so hopefully it's not too experimental.

Polite disagree there. It is very experimental in its API, its usage, and its extensibility model. The code itself is sound and works for its use-case, but this Actor is not "python support in Genny."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think documenting is better than not documenting. That said, what would you think of starting with

## Creating a Python Actor

Please do not create a Python Actor. 

EXISTING TEXT

In my head I'm saying I'll back off from the discussion on highly while then offering counter arguments :) We all agree it's experimental. The modifiers that go before it aren't so important one way or the other.

Copy link
Contributor

@dalyd dalyd Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I want to avoid is someone adding a python actor to test something like an aggregation pipeline or otherwise actually drives load against the system under test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some changes and sprinkled in some bold text 😀. I tried to make the pros/cons/limitations more clear so that test authors can draw good conclusions. Let me know if you think there are more pros/cons/limitations we should capture.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to add one more caveat. Something like the below. Please let me know if unclear.


If you take this path, for the love of $deity, avoid adding imports for packages other than the stdlib and requests. Don't rely on your cwd, the calling parameters, or any other environmental invariants.

Python Actors currently share Genny's venv.
Genny reserves the right to update its dependencies in incompatible ways that might break your Actor's python. If this happens, we'll let you know, but we'll then comment out or remove your Actor for you to fix it. Moreover we would like to avoid spinning up a new interpreter for each loop. We are still thinking on allowing Actors to specify their own Python requirements and a cleaner interaction model.

This facility is in its infancy, and you should expect to have to rewrite any Python at least once and probably not at an opportune time.

Copy link
Contributor

@dalyd dalyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. We'll see what Ryan thinks.

Could you line wrap the new text?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants