Skip to content

Got the services up and running #6

Merged
CasperWA merged 6 commits intomasterfrom
fix-services
Jan 26, 2022
Merged

Got the services up and running #6
CasperWA merged 6 commits intomasterfrom
fix-services

Conversation

@jesper-friis
Copy link
Copy Markdown
Contributor

@jesper-friis jesper-friis commented Jan 23, 2022

  • use a branch in oteapi-core with a quickfix that skips await in datacache
  • improve Dockerfile:
    • only copy what is needed into the docker image
    • added some commented out code that installs oteapi-core such that it can be debugged while the docker is running
  • removed some old files

@jesper-friis jesper-friis marked this pull request as draft January 23, 2022 22:40
Comment thread Dockerfile
# RUN pytest --cov app

# For debugging: install oteapi locally
#COPY oteapi-core oteapi-core
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.

The local oteapi-core should not be inside oetapi-services. Copy from path to is my suggstion.

Suggested change
#COPY oteapi-core oteapi-core
# copy oteapi-core from local path
#COPY ../oteapi-core oteapi-core

Furthermore, this does not allow for runing services locally while developing oteapi-core. That would require mounting the oteapi-core as a volume and pip installing from there.

I have tried to do that but right now the services fail as soon as there is an error, making everything stop.

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.

Agree, placing oteapi-core inside oteapi-services is not the correct way to do things. It is just a hack that makes it much easier to debug issues within oteapi-core that makes the services crash.

Comment thread Dockerfile

# For debugging: install oteapi locally
#COPY oteapi-core oteapi-core
#RUN pip install -U /app/oteapi-core
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.

Installing for development of oteapi-core needs pip install -e . on volume mounted.

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.

Tried that, but that didn't worked for me.

This is just a hack that makes it easier to debug issues related to oteapi-core. It should not be used in production code which is why it is commented out in this PR.

I agree that it should be removed as soon as we have a better way to debug.

@jesper-friis jesper-friis requested a review from CasperWA January 25, 2022 08:17
@jesper-friis jesper-friis requested a review from quaat January 25, 2022 12:54
@jesper-friis jesper-friis marked this pull request as ready for review January 25, 2022 13:06
Comment thread Dockerfile Outdated
Comment thread requirements.txt Outdated
numpy==1.22.1
openpyxl==3.0.9
oteapi-core @ git+https://github.com/EMMC-ASBL/oteapi-core.git@4a7f087e464d9ac017aeb5e5a96be659e4816176
oteapi-core @ git+https://github.com/EMMC-ASBL/oteapi-core.git@90961930b7c1f83466c28a7e03dc04122a98e812
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.

Couldn't we point to master instead?

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.

Good point!

Copy link
Copy Markdown
Contributor

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Cool - let's get it in

@CasperWA CasperWA merged commit 775b9fa into master Jan 26, 2022
@CasperWA CasperWA deleted the fix-services branch January 26, 2022 08:51
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.

3 participants