Skip to content

Fix Docker Build Errors and Refactor the code in src/flask.py #189

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Shoaib-Programmer
Copy link

@Shoaib-Programmer Shoaib-Programmer commented Apr 5, 2025

Hi there πŸ‘‹

I'm an online student currently working through the CS50 series β€” almost done with CS50x, CS50P, and CS50W. Out of curiosity, I decided to check out cs50/python-cs50, so I cloned the repo and tried running docker compose build, as shown in the README.

However, the build failed due to missing system dependencies required for installing the mysqlclient and psycopg2-binary Python packages. To fix this, I updated the Dockerfile to include the following packages:

  • build-essential
  • pkg-config
  • libpq-dev

After that, the image built successfully! πŸŽ‰

I then ran docker compose up β€” both MySQL and PostgreSQL servers started as expected. I used docker exec -it python-cs50 bash -l to get into the container, and ran the test commands listed in the README. Everything worked fine, except the final command, which tries to commit without an active transaction. I might open a separate issue for that later.

Also, while reading the docker build logs, I saw this warning:

WARN[0000] /home/shoaib-quantumcalc/python-cs50/docker-compose.yml: the attribute `version` is obsolete, it will be ignored, please remove it to avoid potential confusion

So I removed the version line from docker-compose.yml, rebuilt the image, and the warning went away.

Another fix:
I updated the src/flask.py file to use importlib.util.find_spec("flask") instead of pkgutil.get_loader, as the latter is getting deprecated in Python 3.14, and I was afraid future students may not be able to use this library anymore.

I initially saw this error when I was running pytest in my project. It said:

../../python/lib/python3.12/site-packages/cs50/flask.py:39
  /home/shoaib-quantumcalc/python/lib/python3.12/site-packages/cs50/flask.py:39: DeprecationWarning: 'pkgutil.get_loader' is deprecated and slated for removal in Python 3.14; use importlib.util.find_spec() instead
    flask_loader = pkgutil.get_loader("flask")

So I went on and replaced it. Not just that, I've refactored the code quite a bit in flask.py! For instance, I've removed unused imports, and replaced

except:

with:

except Exception:

to align with best practices.

Thanks for this awesome library β€” it's been fun using it in my projects! It has made it relatively trivial to use it for managing databases compared to other libraries! Hope this PR is helpful. πŸ™Œ

P.S. I also took a course on Docker (via YouTube University πŸ˜„).

@Shoaib-Programmer Shoaib-Programmer changed the title Fix Docker Build Errors Fix Docker Build Errors and Refactor the code in src/flask.py Apr 5, 2025
Copy link
Member

@dmalan dmalan left a comment

Choose a reason for hiding this comment

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

Thank you for this! Best to submit smaller PRs, though, that address individual issues. And the pkgutil code should be tightened up!

_wrap_flask(module) # Pass the loaded module object to _wrap_flask

# Monkey-patch the loader's exec_module method with our wrapper
flask_spec.loader.exec_module = _exec_module_after
Copy link
Member

Choose a reason for hiding this comment

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

@rongxin-liu mind reviewing?

@Shoaib-Programmer
Copy link
Author

@dmalan thank you for reviewing my PR! My bad for such a cluttered PR! (I'm new to this) I resolved most of the conversations, and a couple merge conflicts with main. I'm eager for @rongxin-liu's reviewing!

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.

2 participants