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

35: Move orchestrator db schema here and rewire integration test to u… #36

Conversation

lfse-slafleur
Copy link
Contributor

@lfse-slafleur lfse-slafleur commented Apr 30, 2024

…se computation engine as a submodule.

Needs to be coordinated with Project-OMOTES/omotes-system#43

…strator/ to make the docker compose project more descriptive and add a first version of README. Also make some alembic scripts available to work with database revisions.
…ake a relevant example and committing it here. Adds unit tests for the new_job_submitted_handler in main.py:Orchestrator
@lfse-slafleur lfse-slafleur marked this pull request as ready for review May 6, 2024 12:20
Copy link
Contributor

@MarkTNO MarkTNO left a comment

Choose a reason for hiding this comment

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

Again really nice to have these tests available, extremely useful!

A couple of general remarks:

  • There are a lot of console prints during the integration test, this would probably make it harder to track down issues. Is it possible to reduce the prints?
  • Should we divide unit_test/test_main.py into several files?
  • I really appreciate your effort in creating 'win32' cmd files, however I don't use them.. Since two weeks I run everything under WSL (file system and venv) and before I was using 'git bash' for the shell scripts. So as far as I am concerned we can leave out these cmd files. It is much better to develop with the same scripts as will be used on ci.

integration_test/setup.sh Outdated Show resolved Hide resolved
unit_test/test_main.py Outdated Show resolved Hide resolved
@lfse-slafleur
Copy link
Contributor Author

* There are a lot of console prints during the integration test, this would probably make it harder to track down issues. Is it possible to reduce the prints?

Quieted down by setting the log level.

* Should we divide `unit_test/test_main.py` into several files?

I would vote not yet. This PR is quite big already. Also, a large test file is usually a sign the 'file under test' (main.py) is too big and should be split up. In other words, if we split up main.py, the unit test file will be reduced as well. But I would vote to do this later.

* I really appreciate your effort in creating 'win32' cmd files, however I don't use them.. Since two weeks I run everything under WSL (file system and venv) and before I was using 'git bash' for the shell scripts. So as far as I am concerned we can leave out these cmd files. It is much better to develop with the same scripts as will be used on ci.

Michiel requires them.

@lfse-slafleur
Copy link
Contributor Author

Also, there remains an issue that sometimes causes the integration test to fail. It is an issue within aio-pika. I have captured the issue here mosquito/aio-pika#630 and will solve it myself. Unsure when it gets solved and merged in. Should not be an issue on production systems as the main reason it fails in the integration test is because the jobs finish so extremely fast. Jobs within omotes takes at least a couple of seconds at the moment. Should be fixed ASAP (high priority). Reminder captured here: #37

@MarkTNO
Copy link
Contributor

MarkTNO commented Jun 3, 2024

Thanks for resolving!
One more question about the win cmd files. Would Michiel be able to work with the bash files in git bash or powershell?

If not, should we provide cmd equivalents for all scripts? For instance the integration_test/ci scripts? And all scripts in the computation engine?

@lfse-slafleur
Copy link
Contributor Author

Thanks for resolving! One more question about the win cmd files. Would Michiel be able to work with the bash files in git bash or powershell?

If not, should we provide cmd equivalents for all scripts? For instance the integration_test/ci scripts? And all scripts in the computation engine?

I am not sure exactly what his current working process is but I know he can work with cmd and ps files.
Eventually when we uses the other scripts, I propose we port them then. For now, I ported the 'easy' ones already.

@lfse-slafleur lfse-slafleur merged commit d1f184b into main Jun 4, 2024
13 checks passed
@lfse-slafleur lfse-slafleur deleted the 35-move-orchestrator-db-schema-to-orchestrator-and-rewire-integration-test-to-use-computation-engine-directly branch June 4, 2024 09:17
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.

Move orchestrator db schema to orchestrator and rewire integration test to use computation engine directly
2 participants