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

Fix: Execution creation was not tested #558

Merged
merged 12 commits into from
Apr 10, 2024
Merged

Fix: Execution creation was not tested #558

merged 12 commits into from
Apr 10, 2024

Conversation

hoh
Copy link
Member

@hoh hoh commented Mar 6, 2024

Solution: Add a test that creates a new VM execution and checks that it starts properly.

@hoh hoh force-pushed the hoh-test-create-vm branch 3 times, most recently from f700d21 to 6319dfd Compare March 6, 2024 16:44
@hoh hoh self-assigned this Mar 7, 2024
Copy link

codecov bot commented Mar 13, 2024

Codecov Report

Attention: Patch coverage is 98.07692% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 43.83%. Comparing base (0a4f75f) to head (e30adef).

Files Patch % Lines
src/aleph/vm/guest_api/__main__.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #558      +/-   ##
==========================================
+ Coverage   35.44%   43.83%   +8.38%     
==========================================
  Files          54       55       +1     
  Lines        4900     4944      +44     
  Branches      585      585              
==========================================
+ Hits         1737     2167     +430     
+ Misses       3140     2658     -482     
- Partials       23      119      +96     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hoh hoh force-pushed the hoh-test-create-vm branch 2 times, most recently from 45cb904 to 13ad705 Compare March 13, 2024 15:42
@hoh hoh marked this pull request as ready for review March 13, 2024 16:08
@hoh hoh requested a review from nesitor March 13, 2024 16:14
@github-actions github-actions bot added the BLACK This PR has critical implications and must be reviewed by a senior engineer. label Mar 13, 2024
@aleph-im aleph-im deleted a comment from github-actions bot Mar 14, 2024
@hoh hoh marked this pull request as draft March 26, 2024 14:08
@hoh
Copy link
Member Author

hoh commented Mar 26, 2024

I managed to get the VM working, only one blocking issue left:

# FIXME: This import fails to work in a VM when using pytest
from aleph.sdk.chains.remote import RemoteAccount

@Antonyjin
Copy link
Member

Is there a way to test this locally? I'm trying with hatch run testing:test but it doesn't work.

@hoh
Copy link
Member Author

hoh commented Mar 28, 2024

I added doc to run the tests in TESTING.md https://github.com/aleph-im/aleph-vm/blob/hoh-test-create-vm/TESTING.md

TESTING.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@olethanh olethanh left a comment

Choose a reason for hiding this comment

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

Add a few place were more documentation is needed. Otherwise look good to me, a part from the problem in the example that still fail. And also we should be able to disable test requiring root

.github/workflows/test-on-droplets-matrix.yml Show resolved Hide resolved
tests/supervisor/test_execution.py Show resolved Hide resolved
tests/supervisor/test_execution.py Show resolved Hide resolved
tests/supervisor/test_execution.py Show resolved Hide resolved
TESTING.md Show resolved Hide resolved
hoh and others added 6 commits April 10, 2024 15:38
Solution: Accept a `pathlib.Path` as argument and convert as string.
Co-authored-by: Olivier Le Thanh Duong <[email protected]>
This tests that a VM from the aleph.im network can be downloaded and launched.

Co-authored-by: ajin <[email protected]>
Solution: Modify hatch configuration to have the environmnent properly set up using the virtual environment builtin module

https://hatch.pypa.io/1.3/plugins/environment/virtual/
It's a problem surfaced by another

The visible problem was that the new exection test were hanging, inside the runtime, during the import at the line
 from aleph.sdk.chains.remote import RemoteAccount

after some more investigative work, it was pin pointed to an inner import of eth_utils module (specifically eth_utils.network )

Second problem that made the first visible: in the runtime the pre-compiled bytecode, created during runtime creation in create_disk_image.sh was not used, which made the import of module slower. This surfaced the first problem. The cause of that second problem was that the init1.py code which run the user caude was not launched with the same optimization level as the pre-compiled bytecode and thus recompiled everything. (this is specified in the init1.py #! sheebang on the first line)

Solution: Compile the bytecode with the same optimisation level (-o 2 )
as during run

We haven't found out yet why the eth_utils.network import hang when it
is not precompiler. But this fix the test hanging issue
…asyncio'

Fix: async_sessionmaker was introduced in sqlachemy 2.0, ensure we have at least this version
otherwhise it was using a older system package
Copy link
Collaborator

@olethanh olethanh left a comment

Choose a reason for hiding this comment

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

LGTM now

@olethanh olethanh marked this pull request as ready for review April 10, 2024 15:24
@olethanh olethanh merged commit fe2e74f into main Apr 10, 2024
21 checks passed
Copy link

Failed to retrieve llama text: POST 504:

504 Gateway Time-out


The server didn't respond in time.

@Psycojoker Psycojoker deleted the hoh-test-create-vm branch July 24, 2024 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BLACK This PR has critical implications and must be reviewed by a senior engineer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants