Skip to content

Changed Dockerfile and pyproject.toml to fix docker build issues#269

Merged
peterdudfield merged 4 commits intoopenclimatefix:mainfrom
hrsvrn:main
Mar 20, 2025
Merged

Changed Dockerfile and pyproject.toml to fix docker build issues#269
peterdudfield merged 4 commits intoopenclimatefix:mainfrom
hrsvrn:main

Conversation

@hrsvrn
Copy link
Copy Markdown
Contributor

@hrsvrn hrsvrn commented Mar 17, 2025

Pull Request

Description

This pull request addresses issues with the Docker build process and dependency management in the Quartz Solar Forecast project. The following changes have been made:

  1. Modified the Dockerfile:

    • Removed the pip install -r requirements.txt command
    • Updated the dependency installation process to use pyproject.toml
  2. Updated pyproject.toml:

    • Moved optional dependencies into the main dependencies section to ensure all necessary packages are installed during the Docker build

These changes aim to resolve the Docker build failures and streamline the dependency management process.

Fixes #262

How Has This Been Tested?

  • Yes

I have tested these changes by:

  1. Building the Docker image using the updated Dockerfile
  2. Verifying that the build process completes successfully without any missing dependency errors
  3. Running the container and confirming that the application starts without any import errors

If your changes affect data processing, have you plotted any changes? i.e. have you done a quick sanity check?

  • Yes

Checklist:


@hrsvrn
Copy link
Copy Markdown
Contributor Author

hrsvrn commented Mar 19, 2025

Hi @peterdudfield,
I noticed my pull request has successfully passed the GitHub workflow checks. Would you have a moment to review and possibly merge it when convenient?

I'm curious about the workflow taking approximately 20 minutes to complete. It seems the Hugging Face model download might be contributing to this duration. Perhaps implementing model caching could be beneficial for improving efficiency in the future?

pyproject.toml Outdated
"pydantic_settings",
"httpx",
"sentry_sdk"
"sentry_sdk",
Copy link
Copy Markdown
Contributor

@peterdudfield peterdudfield Mar 20, 2025

Choose a reason for hiding this comment

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

why did you do this? I think its nice to split the dependencies, then the user can select which requirements they want to install, for example pip install -e .[all] i think installs all of them

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@peterdudfield I've made the modifications to the Docker configuration and updated the pyproject.toml file according to your requirements. I've used the pip installation with the "all" tag since the API requires FastAPI and other dependencies that weren't working with the standard installation.

Lemme know anything else is required!

pyproject.toml Outdated
warn-unused-ignores = true
show-error-codes = true
warn-unreachable = true
warn-unreachable = true No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

could you role back this change, i think it needs an extra line at the end of the file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

image
Like this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i think so

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think i have fixed it. Please review the changes once more!I believe I've resolved the issue. Would you kindly review the changes once more to confirm?


# Install the quartz_solar_forecast package in editable mode
RUN pip install -e .
RUN pip install -e .[all]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this looks good

pyproject.toml Outdated
warn-unused-ignores = true
show-error-codes = true
warn-unreachable = true
warn-unreachable = true No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i think so

@hrsvrn hrsvrn requested a review from peterdudfield March 20, 2025 14:04
@peterdudfield
Copy link
Copy Markdown
Contributor

Ready to merge @hrsvrn ?

@hrsvrn
Copy link
Copy Markdown
Contributor Author

hrsvrn commented Mar 20, 2025

yep sure!

@peterdudfield peterdudfield merged commit 9eb6cbb into openclimatefix:main Mar 20, 2025
4 checks passed
@peterdudfield
Copy link
Copy Markdown
Contributor

@all-contributors please add @hrsvrn for infra

1 similar comment
@peterdudfield
Copy link
Copy Markdown
Contributor

@all-contributors please add @hrsvrn for infra

@allcontributors
Copy link
Copy Markdown
Contributor

@peterdudfield

I've put up a pull request to add @hrsvrn! 🎉

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.

The Docker file is broken and has no proper documentation as well

2 participants