-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Added basic profiling #255
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
Changes from 24 commits
39c2106
a935940
7e66d50
df7bbfe
d589255
94688ed
6171cd7
bba32aa
a904b8d
bdef152
e2539cd
6ab427e
fa60827
32ca751
d523f71
c2896f3
e178d38
21c7b8e
54b8844
692770c
2df1eb6
8d1936f
f37d96d
cc43a8c
cf51555
d2fccc1
a96daef
316f2f3
0268df2
ea879b2
b397f9e
c431e7c
5609bbc
0e68019
0f0e34e
7cc5607
4dac950
9a9ea75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,129 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name: Profiling Comparison for Specific File 2 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| on: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pull_request_target: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| types: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - opened | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - reopened | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - synchronize | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| jobs: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| profiler: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| runs-on: ubuntu-latest | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| steps: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Checkout the code from the repository with full history | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Checkout code | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uses: actions/checkout@v3 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fetch-depth: 0 # Fetch all history so we can checkout any commit | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Check if the sender is a maintainer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: check_permissions | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uses: actions/github-script@v6 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| script: | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const sender = context.payload.sender.login; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { data: membership } = await github.rest.orgs.getMembershipForUser({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| org: context.repo.owner, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| username: sender, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }).catch(() => ({ data: { role: null } })); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return membership.role; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Set is_maintainer variable | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "is_maintainer=${{ steps.check_permissions.outputs.result == 'admin' || steps.check_permissions.outputs.result == 'maintainer' }}" >> $GITHUB_ENV | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Stop if not a maintainer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if: env.is_maintainer != 'true' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "User ${{ github.event.sender.login }} is not a maintainer. Exiting." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| exit 0 # Use exit 0 to mark the job as successful but stop execution | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+30
to
+38
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve shell script security and error handling The maintainer check implementation has potential security issues: - name: Set is_maintainer variable
run: |
- echo "is_maintainer=${{ steps.check_permissions.outputs.result == 'admin' || steps.check_permissions.outputs.result == 'maintainer' }}" >> $GITHUB_ENV
+ is_maintainer="${{ steps.check_permissions.outputs.result == 'admin' || steps.check_permissions.outputs.result == 'maintainer' }}"
+ echo "is_maintainer=${is_maintainer}" >> "$GITHUB_ENV"
- name: Stop if not a maintainer
if: env.is_maintainer != 'true'
run: |
+ echo "::warning::Profiling skipped - User ${{ github.event.sender.login }} is not a maintainer"
echo "User ${{ github.event.sender.login }} is not a maintainer. Exiting."
exit 0📝 Committable suggestion
Suggested change
🧰 Tools🪛 actionlint (1.7.4)35-35: shellcheck reported issue in this script: SC2086:info:1:140: Double quote to prevent globbing and word splitting (shellcheck) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Set up Python environment | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Set up Python | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uses: actions/setup-python@v4 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| python-version: '3.10' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Install Poetry | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uses: snok/install-poetry@v1.3.2 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| virtualenvs-create: true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| virtualenvs-in-project: true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| installer-parallel: true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Install dependencies | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| poetry install --no-interaction --all-extras | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| poetry run pip install pyinstrument | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+46
to
+56
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve dependency management security and efficiency The current setup could be more secure and efficient: - name: Install Poetry
uses: snok/install-poetry@v1.3.2
with:
virtualenvs-create: true
virtualenvs-in-project: true
installer-parallel: true
+ version: 1.7.1 # Pin to specific version
- name: Install dependencies
run: |
+ # Verify poetry.lock is in sync
+ poetry lock --check
- poetry install --no-interaction --all-extras
+ # Install only needed dependencies
+ poetry install --no-interaction --only main,dev
poetry run pip install pyinstrument📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Set environment variables for SHAs | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Set environment variables | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "BASE_SHA=${{ github.event.pull_request.base.sha }}" >> $GITHUB_ENV | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "HEAD_SHA=${{ github.event.pull_request.head.sha }}" >> $GITHUB_ENV | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+60
to
+64
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve environment variables setup The current setup lacks validation and has potential shell scripting issues. - name: Set environment variables
run: |
+ if [[ -z "${{ github.event.pull_request }}" ]]; then
+ echo "Error: No pull request context found"
+ exit 1
+ fi
- echo "BASE_SHA=${{ github.event.pull_request.base.sha }}" >> $GITHUB_ENV
- echo "HEAD_SHA=${{ github.event.pull_request.head.sha }}" >> $GITHUB_ENV
+ echo "BASE_SHA=${{ github.event.pull_request.base.sha }}" >> "$GITHUB_ENV"
+ echo "HEAD_SHA=${{ github.event.pull_request.head.sha }}" >> "$GITHUB_ENV"📝 Committable suggestion
Suggested change
🧰 Tools🪛 actionlint (1.7.4)37-37: shellcheck reported issue in this script: SC2086:info:1:62: Double quote to prevent globbing and word splitting (shellcheck) 37-37: shellcheck reported issue in this script: SC2086:info:2:62: Double quote to prevent globbing and word splitting (shellcheck) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Run profiler on the base branch | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Run profiler on base branch | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BASE_SHA: ${{ env.BASE_SHA }} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "Profiling the base branch for code_graph_pipeline.py" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "Checking out base SHA: $BASE_SHA" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| git checkout $BASE_SHA | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "This is the working directory: $PWD" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Ensure the script is executable | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| chmod +x cognee/api/v1/cognify/code_graph_pipeline.py | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Run Scalene | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| poetry run pyinstrument --renderer json -o base_results.json cognee/api/v1/cognify/code_graph_pipeline.py | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Run profiler on head branch | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Run profiler on head branch | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| HEAD_SHA: ${{ env.HEAD_SHA }} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "Profiling the head branch for code_graph_pipeline.py" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "Checking out head SHA: $HEAD_SHA" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| git checkout $HEAD_SHA | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "This is the working directory: $PWD" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Ensure the script is executable | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| chmod +x cognee/api/v1/cognify/code_graph_pipeline.py | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Run Scalene | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| poetry run pyinstrument --renderer json -o head_results.json cognee/api/v1/cognify/code_graph_pipeline.py | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Compare profiling results | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Compare profiling results | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| python -c ' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with open("base_results.json") as f: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| base = json.load(f) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with open("head_results.json") as f: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| head = json.load(f) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cpu_diff = head.get("total_cpu_samples_python", 0) - base.get("total_cpu_samples_python", 0) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| memory_diff = head.get("malloc_samples", 0) - base.get("malloc_samples", 0) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with open("profiling_diff.txt", "w") as f: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f.write(f"CPU Usage Difference: {cpu_diff}\\n") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f.write(f"Memory Usage Difference: {memory_diff} bytes\\n") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with open("profiling_diff.txt", "w") as f: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f.write(f"Error comparing profiling results: {e}\\n") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Post results to the pull request | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can upload this diff result as artifact -> https://github.com/actions/upload-artifact |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| - name: Post profiling results to PR | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| uses: actions/github-script@v6 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| script: | | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const fs = require('fs'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const diff = fs.readFileSync('profiling_diff.txt', 'utf-8'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| github.rest.issues.createComment({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| issue_number: context.issue.number, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| owner: context.repo.owner, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| repo: context.repo.repo, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| body: `### Profiling Results for code_graph_pipeline.py\n\`\`\`\n${diff || 'No differences found.'}\n\`\`\`` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Critical Security Risk: Unsafe usage of pull_request_target
The current trigger configuration using
pull_request_targetposes a significant security risk as it:Implement these security measures:
If
pull_request_targetis absolutely necessary for token permissions:📝 Committable suggestion