Perf checker #1840
Perf checker #1840
Conversation
beroy
force-pushed
the
perf_checker
branch
2 times, most recently
from
36ffb65 to
d673b9f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅ ❗ Your organization needs to install the Codecov GitHub app to enable full functionality. see 75 files with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
profiler/ann_data.py
Outdated
| import tiledbsoma as soma | ||
|
|
||
| census_S3_latest = dict(census_version="latest") | ||
| census_local_copy = dict(uri="/Users/brobatmili/projects/census_data/") |
There was a problem hiding this comment.
rm user path, will require accessing on S3. ideally, to eliminate network variability impact on profiling, the census should be copied locally first, but I don't think host will have sufficient disk space and it's not worth copying the whole census for the single eye tissue-based test. So S3 access is probably best for now, but should deal with network variability by averaging a few runs (maybe that could be built into the profiler as a feature)
There was a problem hiding this comment.
since query times can change with census release versions, due to ever increasing data sizes and schema changes, all profiling comparisons need to be done against a single census data release. So the data release should be fixed. It should be updated on a reasonable schedule, such as when a new LTS data release is made available.
.github/workflows/profiler.yml
Outdated
| - "main" | ||
|
|
||
| paths: | ||
| - ".github/workflows/profiler" |
There was a problem hiding this comment.
was this intended to be indented under on?
profiler/ann_data.py
Outdated
|
|
||
| def main(): | ||
| t1 = perf_counter() | ||
| with cellxgene_census.open_soma(**census_local_copy) as census: |
There was a problem hiding this comment.
I don't think we should use Census API here. We can do the equivalent with just TileDBSOMA API.
profiler/setup.py
Outdated
| @@ -4,5 +4,5 @@ | |||
| name="soma-profiler", | |||
| version="1.0", | |||
| packages=find_packages(), | |||
| requires=["gitpython", "psutil"], | |||
| requires=["gitpython", "comacore", "psutil", "tiledbsoma", "cellxgene_census"], | |||
There was a problem hiding this comment.
somacore. Though that shouldn't be needed since tiledbsoma is required (transitive dependency)
profiler/top_profiler.py
Outdated
|
|
||
| db = data.FileBasedProfileDB() | ||
| actual_max_ts = 0 | ||
| dt = db.find("python ann_data.py") |
There was a problem hiding this comment.
Would parameterize the profile run name(s) to run this. Only perf_checker.sh should know what profiling scripts are being run.
profiler/top_profiler.py
Outdated
There was a problem hiding this comment.
Would rename to profile_report.py, since top-like behavior implies that is monitoring the profiler while it runs.
profiler/top_profiler.py
Outdated
| last_two = dt[-2:] | ||
| c = 0 | ||
|
|
||
| for s in dt: |
There was a problem hiding this comment.
| for s in dt: | |
| for s in last_two: |
I think this is the intention...
profiler/top_profiler.py
Outdated
| L[1] = dt[1].user_time_sec | ||
| for i in range(0, len(dt)): | ||
| print(f"{i} dt[{i}].user_time_sec = {dt[i].user_time_sec} ts {dt[i].timestamp}") | ||
| print(f"L0 = {L[0]} L1 {L[1]}") |
There was a problem hiding this comment.
| print(f"L0 = {L[0]} L1 {L[1]}") | |
| print(f"Prev = {L[0]}, Curr = {L[1]}") |
profiler/top_profiler.py
Outdated
|
|
||
| L = [1, 2] | ||
| L[0] = dt[0].user_time_sec | ||
| L[1] = dt[1].user_time_sec |
There was a problem hiding this comment.
maybe elapsed time? or both.
profiler/top_profiler.py
Outdated
| for s in dt: | ||
| new_db = sorted(dt, key=lambda ProfileData: ProfileData.timestamp) | ||
|
|
||
| L = [1, 2] |
There was a problem hiding this comment.
| L = [1, 2] | |
| L = [] |
also, no need to capitalize
profiler/top_profiler.py
Outdated
|
|
||
| if threshold * float(L[1]) < float(L[0]) or float(L[1]) > threshold * float(L[0]): | ||
| raise SystemExit(f"Potential performance degradation detected {L[0]} va {L[1]}") | ||
| print("No recent performance degradation detected") |
There was a problem hiding this comment.
report should also output the previous and current tiledbsoma versions
There was a problem hiding this comment.
A few lower level comments. But per recent meeting, consensus is that we should avoid running this on GHA at all, unless we can do so on an instance with consistent specs. Also, the more expensive the operations that are being profiled, the more informative the results will be. So we might consider GHA large runners, if we can get work the paid-plan details between the CZI and TileDB organizations.
|
I think we may still have the spec issue with GHA large instances. Maybe infra can help by providing us with a fixed instance. Another possible option is to use an AWS bare metal instance |
|
Closing the profiler harness will go to the census repo |
|
Noting the above
-- closing this PR |
Early version of the profiler.
Changes: #1607
Notes for Reviewer: