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

chore: Add github workflow and script to check python depencies source #34549

Merged

Conversation

salman2013
Copy link
Contributor

@salman2013 salman2013 commented Apr 19, 2024

Description: Add github action to run the python script from repo tools, which finds the python dependencies source and displays them if they belong to any third-party sources.

Resolves #33189

@salman2013 salman2013 marked this pull request as ready for review April 22, 2024 06:28
Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

I think it makes sense for the workflow to live in this repo but the find_python_dependencies.py code should live in the repo_tools directory.

- name: Set up Python
uses: actions/setup-python@v4
with:
python-version: '3.8'
Copy link
Contributor

Choose a reason for hiding this comment

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

Just start with 3.12 for this script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intentionally did that because i believe we have not added support of 3.12 in edx-platform yet,
if we try to update the python-version to 3.12 it results error

Traceback (most recent call last): File "/home/runner/work/edx-platform/edx-platform/scripts/find_python_dependencies.py", line 8, in <module> import requirements File "/opt/hostedtoolcache/Python/3.[12](https://github.com/openedx/edx-platform/actions/runs/8811123770/job/24184575317?pr=34549#step:10:13).3/x64/lib/python3.12/site-packages/requirements/__init__.py", line 19, in <module> from .parser import parse File "/opt/hostedtoolcache/Python/3.12.3/x64/lib/python3.12/site-packages/requirements/parser.py", line 23, in <module> from .requirement import Requirement File "/opt/hostedtoolcache/Python/3.12.3/x64/lib/python3.12/site-packages/requirements/requirement.py", line 24, in <module> from pkg_resources import Requirement as Req ModuleNotFoundError: No module named 'pkg_resources'

The reason for the above error is

gh-95299: Do not pre-install setuptools in virtual environments created with venv. This means that distutils, setuptools, pkg_resources, and easy_install will no longer available by default; to access these run pip install setuptools in the activated virtual environment.

https://docs.python.org/3/whatsnew/3.12.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated it.


- name: Copy Python requirements file
run: |
for req_file in "requirements/edx/base.txt"; do
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think there will be more files in this list than just this one in the future? If so, can you make a list of one item that can be added to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated it.

$ python find_deps.py $OLIVE_DIRS
"""

import concurrent.futures
Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably pare down these imports now that you've updated the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated it.

else:
print(f"Failed to retrieve data for package {package}. Status code:", response.status_code)

def process_directory():
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of processing a directory, why not just pass the path to a file in the main function, then we don't need to do all the directory work, just parse the requirements out of the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure i have updated it.

@salman2013 salman2013 closed this Apr 24, 2024
@salman2013 salman2013 reopened this Apr 24, 2024
@salman2013 salman2013 closed this Apr 25, 2024
@salman2013 salman2013 reopened this Apr 25, 2024
@salman2013 salman2013 closed this Apr 25, 2024
@salman2013 salman2013 reopened this Apr 25, 2024
@salman2013 salman2013 requested a review from feanil May 6, 2024 11:09
Comment on lines 22 to 31
- name: Clone repo-tools repo
uses: actions/checkout@v4
with:
repository: salman2013/repo-tools
path: repo_tools
ref: salman/add-script-for-python-dependencies

- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}

- name: Install setuptool
run: pip install setuptools

- name: Install requests module
run: pip install requests

- name: Install rich module
run: pip install rich

- name: Install requirements parser module
run: pip install requirements-parser
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: Clone repo-tools repo
uses: actions/checkout@v4
with:
repository: salman2013/repo-tools
path: repo_tools
ref: salman/add-script-for-python-dependencies
- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}
- name: Install setuptool
run: pip install setuptools
- name: Install requests module
run: pip install requests
- name: Install rich module
run: pip install rich
- name: Install requirements parser module
run: pip install requirements-parser
- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}
- name: Install repo-tools
run: pip install git+https://github.com/salman2013/repo-tools.git@salman/add-script-for-python-dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@feanil There is one thing in this, i need to install requirements-parser and setuptools so there are two options should i add them here in CI or add them in repo-tool so that the script can find these packages. What do you suggest?

Comment on lines 46 to 47
- name: Make the script files executable
run: chmod +x repo_tools/edx_repo_tools/find_dependencies/find_python_dependencies.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: Make the script files executable
run: chmod +x repo_tools/edx_repo_tools/find_dependencies/find_python_dependencies.py

This shouldn't be necessary if we're pip-installing repo-tools.

run: chmod +x repo_tools/edx_repo_tools/find_dependencies/find_python_dependencies.py

- name: Run Python script
run: python repo_tools/edx_repo_tools/find_dependencies/find_python_dependencies.py requirements/edx/base.txt requirements/edx/testing.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
run: python repo_tools/edx_repo_tools/find_dependencies/find_python_dependencies.py requirements/edx/base.txt requirements/edx/testing.txt
# Tracking ignored repos here: https://github.com/openedx/public-engineering/issues/174
run: python repo_tools/edx_repo_tools/find_dependencies/find_python_dependencies.py requirements/edx/base.txt requirements/edx/testing.txt --ignore https://github.com/edx/codejail-includes --ignore https://github.com/edx/braze-client --ignore https://github.com/edx/edx-name-affirmation --ignore https://github.com/mitodl/edx-sga --ignore https://github.com/edx/token-utils --ignore https://github.com/edx/codejail-includes --ignore https://github.com/edx/braze-client --ignore https://github.com/edx/edx-name-affirmation --ignore https://github.com/mitodl/edx-sga --ignore https://github.com/edx/token-utils

I've made sure that all these repos are part of a tickte to further investigate and clean them up here: openedx/public-engineering#174 so we can ignore them for now and prevent future additions, we'll make sure to remove the repos from this list as they get removed from edx-platform.

@salman2013 salman2013 closed this May 13, 2024
@salman2013 salman2013 reopened this May 13, 2024
@salman2013 salman2013 closed this May 13, 2024
@salman2013 salman2013 reopened this May 13, 2024
run: pip install setuptools

- name: Run Python script
run: find_python_dependencies --dirs requirements/edx/base.txt --dirs requirements/edx/testing.txt --ignore https://github.com/edx/codejail-includes --ignore https://github.com/edx/braze-client --ignore https://github.com/edx/edx-name-affirmation --ignore https://github.com/mitodl/edx-sga --ignore https://github.com/edx/token-utils --ignore https://github.com/edx/codejail-includes --ignore https://github.com/edx/braze-client --ignore https://github.com/edx/edx-name-affirmation --ignore https://github.com/mitodl/edx-sga --ignore https://github.com/edx/token-utils
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, can you split this over multiple lines so that it's easier to read?

@salman2013 salman2013 requested a review from a team as a code owner May 20, 2024 07:34
@salman2013
Copy link
Contributor Author

@feanil I think as per the documentation references, we should explicitly install the setuptool in environment where needed, like in our case I think in CI its best suited place instead of including it in repo-tool base.txt

python/cpython#95299
https://peps.python.org/pep-0632/

@salman2013 salman2013 closed this Jun 4, 2024
@salman2013 salman2013 reopened this Jun 4, 2024
@salman2013 salman2013 closed this Jun 4, 2024
@salman2013 salman2013 reopened this Jun 4, 2024
@salman2013 salman2013 closed this Jun 5, 2024
@salman2013 salman2013 reopened this Jun 5, 2024
@salman2013 salman2013 closed this Jun 5, 2024
@salman2013 salman2013 reopened this Jun 5, 2024
@salman2013 salman2013 closed this Jun 5, 2024
@salman2013 salman2013 reopened this Jun 5, 2024
@salman2013 salman2013 force-pushed the salman/add-ci-check-python-dependencies branch from 5628968 to 87a7e87 Compare June 5, 2024 12:05
@salman2013 salman2013 requested a review from feanil June 6, 2024 11:25
Comment on lines 27 to 34
- name: Install repo-tools
run: pip install git+https://github.com/salman2013/repo-tools.git@salman/add-script-for-python-dependencies

- name: Install requirements parser module
run: pip install requirements-parser

- name: Install setuptool
run: pip install setuptools
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: Install repo-tools
run: pip install git+https://github.com/salman2013/repo-tools.git@salman/add-script-for-python-dependencies
- name: Install requirements parser module
run: pip install requirements-parser
- name: Install setuptool
run: pip install setuptools
- name: Install repo-tools
run: pip install edx-repo-tools[find_dependencies]

I think this is all you should need now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@feanil repo-tool is requiring setuptool to run this github action on 3.12 so we have two options either we should keep it here like following step or install it in repo-tool but I believe it should be here where we need it as python has dropped its default support in 3.12

  • name: Install setuptool
    run: pip install setuptools

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated other things and everything is working fine.

@salman2013 salman2013 requested a review from feanil June 10, 2024 12:17
@feanil feanil merged commit 39a1369 into openedx:master Jun 10, 2024
46 checks passed
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

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.

Add CI check for dependencies from the edx GitHub org
3 participants