Skip to content

feat: add support for Remote Cloud Development with Gitpod #23556

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

Closed
wants to merge 1 commit into from
Closed

Conversation

Siddhant-K-code
Copy link

@Siddhant-K-code Siddhant-K-code commented Jun 7, 2022

👋🏼 Hello, Siddhant here from Gitpod

Reference Issues/PRs

N/A

What does this implement/fix? Explain your changes.

Problem: It would skip all the pain in running the manual steps for setup & building from the source, running all the manual commands with Hardware limitations.

Solution: it Contains a .gitpod.yml file which runs all the necessary commands (docs) required to create a ready to code environment on Gitpod. It would help contributors to directly jump into the main part without doing the manually running the Installation commands.

Any other comments?

To Test it out, Just Click on following button & see the preview:

Open scikit-learn in Gitpod

@lesteve
Copy link
Member

lesteve commented Jun 7, 2022

I would have appreciated that you clearly disclosed that you are currently working for Gitpod in the description of this PR.

I see that you have opened similar PRs in other projects. I would personally give a very similar answer to the Ansible maintainer in ansible/ansible#77942 (comment): this is not something I am keen on maintaining going forward.

More than happy to hear other maintainers opinion about this one!

@lesteve lesteve added the Needs Decision - Include Feature Requires decision regarding including feature label Jun 7, 2022
@Siddhant-K-code
Copy link
Author

I would have appreciated that you clearly disclosed that you are currently working for Gitpod in the description of this PR.

Yeah, forgot to mention that with current PR template. Updated it now, sorry for that.

I see that you have opened similar PRs in other projects. I would personally give a very similar answer to the Ansible maintainer in ansible/ansible#77942 (comment): this is not something I am keen on maintaining going forward.

I respect your opinion, but, this .gitpod.yml file would be the same till we didn't change the build steps (which would be for the long time). I think, there won't be a more change to this file.

@thomasjpfan
Copy link
Member

thomasjpfan commented Jun 7, 2022

I have done the initial work for adding GitPod support to NumPy. The NumPy config runs on a numpy/numpy-gitpod docker image to have a warm cache of the compiled code using ccache. This makes it faster to get up and runner for users working on a fork. Keep in mind that, building the numpy/numpy-gitpod is more code to maintain. Note, we could not use GitPod's pre-built feature because if the pre-built was built on numpy/numpy a fork such as thomasjpfan/numpy will not use it and have to rebuild it from scratch.

For us to introduce GitPod, there is some documentation work to be done. For example, NumPy has a whole page on using GitPod, and they have a FAQ describing common problems.

The biggest benefit I see for this is for running sprints, where we do not need to spend the first hour debugging development environments. For normal development, I think it's good to have users go through the process of installing and testing on different platforms. This helps keep our install instructions up to date for different platforms and helps with debugging for a specific platform.

Overall I am -0.25 for adding GitPod for scikit-learn. In general, GitPod does make it easier to contribute, but there is a balance of contributor time and maintainer time. A maintainer needs to maintain the code and docs related to GitPod and debug issues a user may have with running GitPod for development.

@ogrisel
Copy link
Member

ogrisel commented Jun 8, 2022

I tried to open a VS Code session by clicking on the badge of the description of the PR, and the VS Code terminal has the following error message by default:

HISTFILE=/workspace/.gitpod/cmd-1 history -r; {
[ -r /workspace/.prebuild-log-1 ] && cat /workspace/.prebuild-log-1; [ -r /workspace/.gitpod/prebuild-log-1 ] && cat /workspace/.gitpod/prebuild-log-1; true
} && {
python -c "import sklearn; sklearn.show_versions()"
}
gitpod /workspace/scikit-learn-scikit-learn (main) $  HISTFILE=/workspace/.gitpod/cmd-1 history -r; {
> [ -r /workspace/.prebuild-log-1 ] && cat /workspace/.prebuild-log-1; [ -r /workspace/.gitpod/prebuild-log-1 ] && cat /workspace/.gitpod/prebuild-log-1; true
> } && {
> python -c "import sklearn; sklearn.show_versions()"
> }
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/workspace/scikit-learn-scikit-learn/sklearn/__init__.py", line 82, in <module>
    from .base import clone
  File "/workspace/scikit-learn-scikit-learn/sklearn/base.py", line 13, in <module>
    import numpy as np
ModuleNotFoundError: No module named 'numpy'

Also the VS Code obtained is missing all the useful extensions (e.g. Python tools, Cython syntax highlighting...).

It's probably because we would need to inform vs code to activate the sklearn-env by default which is not the case here.

I share the same feeling as @thomasjpfan and @lesteve in terms of maintenance.

Maybe someone would like to volunteer to maintain a well documented gitpod config for scikit-learn in a side repo. If it works well out of the box for first time contributors at sprints, then we might decide to make this more official and move such a repo under the https://github.com/scikit-learn/ organization later.

But I am also -0.25 to maintain the doc of a gitpod deployment in the main scikit-learn documentation, at least in the short term.

- init: |
python3 -m venv sklearn-env
source sklearn-env/bin/activate
pip3 install wheel numpy scipy cython
Copy link
Member

Choose a reason for hiding this comment

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

Since we have activated the venv, it would make sense to use the pip command directly for consistency with the next line.

Suggested change
pip3 install wheel numpy scipy cython
pip install wheel numpy scipy cython pandas matplotlib

I also added pandas and matplotlib that are used in many examples.

@partev
Copy link
Contributor

partev commented Feb 12, 2023

@Siddhant-K-code I get the following error:

` HISTFILE=/workspace/.gitpod/cmd-1 history -r; {
python -c "import sklearn; sklearn.show_versions()"
}
gitpod /workspace/scikit-learn (main) $ HISTFILE=/workspace/.gitpod/cmd-1 history -r; {

python -c "import sklearn; sklearn.show_versions()"
}
Traceback (most recent call last):
File "/workspace/scikit-learn/sklearn/__check_build/init.py", line 48, in
from ._check_build import check_build # noqa
ModuleNotFoundError: No module named 'sklearn.__check_build._check_build'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
File "", line 1, in
File "/workspace/scikit-learn/sklearn/init.py", line 81, in
from . import __check_build # noqa: F401
File "/workspace/scikit-learn/sklearn/__check_build/init.py", line 50, in
raise_build_error(e)
File "/workspace/scikit-learn/sklearn/__check_build/init.py", line 31, in raise_build_error
raise ImportError(
ImportError: No module named 'sklearn.__check_build._check_build'


Contents of /workspace/scikit-learn/sklearn/__check_build:
init.py _check_build.pyx pycache


It seems that scikit-learn has not been built correctly.

If you have installed scikit-learn from source, please do not forget
to build the package before using it: run python setup.py install or
make in the source directory.

If you have used an installer, please check that it is suited for your
Python version, your operating system and your platform.
gitpod /workspace/scikit-learn (main) $`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Decision - Include Feature Requires decision regarding including feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants