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 codespace issue #46

Merged
merged 3 commits into from
Dec 9, 2024
Merged

Fix codespace issue #46

merged 3 commits into from
Dec 9, 2024

Conversation

Kostis-S-Z
Copy link
Contributor

@Kostis-S-Z Kostis-S-Z commented Dec 6, 2024

What's changing

Currently the .devcontainer.json is ignored because its under the .github directory. It needs to be either in root or .devcontainer/ according to the docs.

This meant that the default configuration of codespaces for a machine with 16GB RAM was ignored, and instead was using the 8GB RAM machine which is not sufficient to properly run the demo app in codespaces. Thanks @ameckes1229 for the discovery!

Also the pre-installation step used in the .devcontainer.json was deprecated and now its updated to use the .github/setup.sh script.

How to test it

Steps to test the changes:

  1. Create default codespace (just by clicking +)
  2. Click Code in the repo -> ... in the active codespace -> Change machine type -> Verify that the machine selected is the 16GB one
  3. Run the demo in codespaces without first installing anything. The installation of the package should already been taken care of automatically when setting up the codespace.

Additional notes for reviewers

Not sure how we can test this before merging to main :/

Edit: The only way to test this (to my understanding) is by using pre-build codespaces, but we need to discuss how or if we can set this up on org level. About testing codespaces configuration changes here

I already...

  • Tested the changes in a working environment to ensure they work as expected
  • Added some tests for any new functionality
  • Updated the documentation (both comments in code and under /docs)

@Kostis-S-Z Kostis-S-Z marked this pull request as draft December 6, 2024 18:49
@Kostis-S-Z Kostis-S-Z marked this pull request as ready for review December 6, 2024 18:51
@Kostis-S-Z Kostis-S-Z self-assigned this Dec 6, 2024
@stefanfrench
Copy link
Contributor

@Kostis-S-Z - i just tried to test this. I may be doing something wrong, but on this branch, I created default codespace and it did the 8GB version, not the 16GB version.

Also, i tried just running python -m streamlit run demo/app.py but got the following error: /home/codespace/.python/current/bin/python: No module named streamlit

@Kostis-S-Z
Copy link
Contributor Author

Kostis-S-Z commented Dec 9, 2024

@Kostis-S-Z - i just tried to test this. I may be doing something wrong, but on this branch, I created default codespace and it did the 8GB version, not the 16GB version.
Also, i tried just running python -m streamlit run demo/app.py but got the following error: /home/codespace/.python/current/bin/python: No module named streamlit

Thanks for testing @stefanfrench ! I am aware that its not working on this branch (thats why I also wrote in the notes of the PR "Not sure how we can test this before merging to main :/"). This is because when you start the codespaces it only looks at main by default to see if there is a configuration, even if you then choose this branch, it will be too late.

I found the fix here. We basically need to enable pre-build codespaces, but when I tried to set it up I got this!
image

I guess we need to look into this and see what the cost would be.

@daavoo
Copy link
Contributor

daavoo commented Dec 9, 2024

image

@daavoo
Copy link
Contributor

daavoo commented Dec 9, 2024

Edit: The only way to test this (to my understanding) is by using pre-build codespaces, but we need to discuss how or if we can set this up on org level. About testing codespaces configuration changes here

I think is ok to merge and test from main to see if this works as expected. pre-build are a different purpose feature (still could be also interesting to look into it)

@daavoo daavoo merged commit db27ce1 into main Dec 9, 2024
3 checks passed
@daavoo daavoo deleted the fix-devcontainer-dir branch December 9, 2024 08:52
@daavoo
Copy link
Contributor

daavoo commented Dec 9, 2024

It looks like these changes had no effect, I tested the default creation from main after merging this PR

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