JoeyHendricks / python-benchmark-harness

A micro/macro benchmark framework for the Python programming language that helps with optimizing your software.
MIT License
154 stars 13 forks source link

Great work! Here are some discussion topics... #9

Closed afparsons closed 2 years ago

afparsons commented 3 years ago

This is a nice library. I had some time to play with it this afternoon (we previously exchanged a few messages on Reddit) and I am impressed.

If you don't mind, I am going to use this GitHub issue to maintain a dialogue with you since it appears that you have not enabled GitHub Discussions. If I develop any concrete feature requests or specific questions or bug reports, I will spin those out as separate and discrete GitHub issues.


I intend to use QuickPotato to find slow parts of my Dagster pipelines.

Dagster describes itself in the following way:

Dagster is a data orchestrator for machine learning, analytics, and ETL

Essentially, Dagster is a competitor/spiritual successor to Apache Airflow. It is used as follows:

I was most attracted to this library because of the intrusive profiling this library offers. The other alternatives I looked at (py-spy, pyflame, etc.) require providing a specific Python script as an argument to the profiler. That approach simply doesn't work when integrating with Dagster! Being able to wrap a function and to know that the function will be profiled whenever it is called (and enabled=True) is really awesome. Congratulations :grinning:

I have provided the context above just so you understand my use cases.

I've watched your NeotysPAC demonstration. Please correct me if I am wrong, but it seems like your initial idea was to build the intrusive profiler, and since then you have subsequently added in the unit test workflows (for example, the pt.measure_method_performance(...) construct). At this point, it seems like the performance_breakpoint decorator is not feature complete.

Below are my thoughts after experimenting with QuickPotato for about an hour. Please note that I have not yet looked through the codebase extensively and I might hold some misconceptions.

QuickPotato/database/operations.py

return f"{self.URL}/{database_name}"



- I would like a way to link a specific execution of a Dagster `Pipeline` to its QuickPotato profile record. Each run of a Dagster `Pipeline` is automatically assigned a UUID, and its metadata, logs, etc. are stored in a database keyed by this UUID. It looks like each QuickPotato benchmark has both a `test_id` and a `sample_id`. It would be great to be able to get those from the `StatisticsInterpreter` before/during/after it writes to the database so I can then match the Dagster and QuickPotato IDs. I don't know how to do that though. Maybe modifying the decorator to work with the observer pattern could work? The `test_id` and `sample_id` could be given to some other function which handles matching the profile statistics with the Dagster `run_id`.

Anyway, great work so far!
JoeyHendricks commented 3 years ago

I am very happy to hear you like QuickPotato and thank you for the compliments this project has been my pandemic hobby project!

Yes, the performance breakpoint was not yet "feature complete" mainly because previous feedback was more like "I don't want to be intrusive in my production code for testing purposes." Out of that, the non-intrusive construct was born because it was not intrusive and would allow for easier use for creating "micro" performance tests for your code.

So I started mainly to see the performance breakpoint as a more advanced feature to better control profiling and use it for custom stuff. I never removed it because it goes against the vision of the framework and was planning of improving it soon.

The vision of QuickPotato is to make profiling more flexible. easier in use for automation purposes and allow developers to write focussed performance tests for their code so performance assessment happens earlier in the development life cycle. I have found these concepts missing in similar frameworks so that is why this project exists.

Do be warned this decorator does add an overhead to your function so for monitoring purposes in production you might be better of using an APM as they provide "somewhat" the same insight against lesser overhead. But if you don't wanna use any fancy APM tooling (They cost a lot of money if you want a decent one.) you can also just flick the decorator on and off using the parameters on specific intervals as a sort of "advanced logging".


After reading what you want to achieve with the "performance breakpoint" I made the following changes to the master branch (This is always my unstable edge version):

All of this would look like this in regular old Python code:

# Example code is not on GitHub this is a simple CRUD app to manually test QuickPotato.
from DummyApp.main import ToDoList
from QuickPotato.profiling.intrusive import performance_breakpoint
from QuickPotato.configuration.management import options
from QuickPotato.statistical.visualizations import FlameGraph, CsvFile, HeatMap, BarChart

# Settings and stuff
options.connection_url = "mysql+pymysql://root:password@localhost"
options.enable_intrusive_profiling = True  # <-- Make sure that profiling is enabled

# Building the class that we will use in the test
instance = ToDoList()

# Parameters used by the break point
rand_test_case_name = "I am a test case"
rand_database_name = " I am a database name"
rand_test_id = "12345678910"

# Decorating the function
@performance_breakpoint(test_case_name=rand_test_case_name, database_name=rand_database_name, test_id=rand_test_id)
def dummy_function():
    instance.add_action_to_board(payload=[{"Action": "Do Something", "Person": "Joey Hendricks", "DueDate": "NOW"}])

# Running the test
for _ in range(0, 10):
    dummy_function()

# Exporting some visuals 
FlameGraph(test_case_name=rand_test_case_name,  database_name=rand_database_name , test_id=rand_test_id ).export("C:\\temp\\")
CsvFile(test_case_name=rand_test_case_name,  database_name=rand_database_name , test_id=rand_test_id ).export("C:\\temp\\")
HeatMap(test_case_name=rand_test_case_name, database_name=rand_database_name , test_ids=[rand_test_id ]).export("C:\\temp\\")
BarChart(test_case_name=rand_test_case_name, database_name=rand_database_name , test_ids=[rand_test_id ]).export("C:\\temp\\")

What bothers me a bit with these changes to the decorator is that this does include a lot of extra parameters which looks a bit ugly and could be better solved by sharing the parameters as a dictionary ;D . What do you think?

As is customary these changes have also created some other bugs and because of that I have already checked most of the framework code base for problems but some probably still exist in the statistical analysis parts of the project. So I will patch up those parts somewhere this week as soon as I have some free time probably this weekend or somewhere next week.


On a side note, as QuickPotato is a testing tool so by default it is packed with analytical capabilities allowing you to compare your dragster runs for regression after code changes and set SLA targets that a dragster run needs to meet to be able to pass your performance test. (Soon I will be adding my own regression algorithm that would make these regression tests more reliable as they now only work through a T-test which works great but can be better.)

These results can then be stored over time by increasing or disabling the data retention policy and can be analyzed after a while to verify if the speed of your product is improving or degrading. (This is information is very powerful and fairly unique to come across in a project.)

Thank you for going through the project I am very happy somebody finds it useful :) cheers from the hilly South of Holland! I have now enabled GitHub discussions but I like issues as they can also double as a bit of documentation for the project :).


Below are the visuals that the example code has generated:

visuals of the example code.zip

afparsons commented 3 years ago

previous feedback was more like "I don't want to be intrusive in my production code for testing purposes."

That's interesting. I see the intrusive profiler as this library's strength and differentiating feature. If someone doesn't want instrusive profiling, then they should use one of the many other sophisticated and mature Python profiling libraries! :smile:

Do be warned this decorator does add an overhead to your function

Noted; the code I wish to profile takes minutes to hours (the last run was 45 hours!) to execute, and so I'm not too concerned with a few added seconds.

What bothers me a bit with these changes to the decorator is that this does include a lot of extra parameters which looks a bit ugly and could be better solved by sharing the parameters as a dictionary ;D . What do you think?

That much is up to you; if you aren't pleased with it, go ahead and roll back the changes. Perhaps set defaults to effectively make providing arguments to the parameters optional? I'm not sure what the best approach is, but I will think about it.

I'm going to play around on my fork with a few ideas I had (keep in mind that I'm thinking of some specific use-cases). If I develop anything I think is worthwhile, then I'll submit a pull request, and we can see if my suggestions fit with your vision.

Lastly, I gave you a shout-out here. If you see an increase in interest, that's why :smile:


cheers from the hilly South of Holland!

I assume you mean Limburg and not the province of South Holland.

hilly

At first I thought you were joking, since the Netherlands are notoriously flat, but after looking at a map, I realized Limburg is quite near the German Eifel. I lived in the Eifel for about eleven months as a foreign exchange student. I don't think I spent any time in Limburg, but I'd imagine that it is geographically similar to the regions immediately eastward. That's a quaint corner of the world :heart: Sorry to hear about the floods.

JoeyHendricks commented 2 years ago

Thank you for the shout out, it means a lot to me! I am ramping up my work again on QuickPotato and will take your feedback into account. After working on my last conference topic I changed jobs because of this change I did not have much free time to continue to improve QuickPotato because of that I am also replying so late (sorry). However most of the stuff I researched for my recent conference topic is also quite relevant for QuickPotato.

You can find some more info here:

https://github.com/JoeyHendricks/automated-performance-test-result-analysis

That is why in my next update I will be introducing the following new things:

If you still want to and have the time the changes you introduced into your fork are great improvement to give people more control over when they want to profile. If you wanna chat further discussion is now open on the repo or shoot me an message on LinkedIn.

Cheers, Joey