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

PR: Do not import spyder before install_repo in bootstrap.py #19838

Merged
merged 1 commit into from
Oct 15, 2022

Conversation

mrclary
Copy link
Contributor

@mrclary mrclary commented Oct 14, 2022

Issue(s) Resolved

Fixes #19832

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: @mrclary

…not already installed. Also, do not assume repository name is "spyder".
@mrclary mrclary requested a review from dalthviz October 14, 2022 22:27
@mrclary mrclary self-assigned this Oct 14, 2022
@mrclary mrclary added this to the v5.4.0 milestone Oct 14, 2022
@mrclary
Copy link
Contributor Author

mrclary commented Oct 14, 2022

/show binder

@github-actions
Copy link

Binder 👈 Launch a Binder instance on this branch

@mrclary
Copy link
Contributor Author

mrclary commented Oct 15, 2022

@ccordoba12 @dalthviz, Spyder now launches in Binder, but there are still a few problems. The following missing dependencies are not installed in the environment and I'm not sure why.
Screen Shot 2022-10-14 at 5 28 31 PM

@mrclary
Copy link
Contributor Author

mrclary commented Oct 15, 2022

Okay, it seems that Binder behaves differently when built from the 5.x branch vs. the master branch. I'm not sure exactly why that is.

The binder link above, in this PR, on 5.x, results in the repository copied to a "spyder" directory in the home directory and missing dependencies in the environment. The shortcut on the desktop is labeled with 5.x.

I cherry-picked this PR's changes onto the master branch in my fork and built the binder from there and found that the repository is copied to the home directory and all dependencies are installed and Spyder launches and runs fine. The shortcut on the desktop is labeled with 4.x.

So I suppose everything is okay if run from the master branch. But it seems that some idiosyncrasies should be cleaned up at some point.

@ccordoba12
Copy link
Member

ccordoba12 commented Oct 15, 2022

Spyder now launches in Binder, but there are still a few problems. The following missing dependencies are not installed in the environment and I'm not sure why.

@mrclary, we've talked about this before but probably you forgot it. So, we create Binder links for PRs through an associated repo to avoid rebuilding the Binder instance on every PR commit, which is very inefficient:

https://github.com/spyder-ide/binder-environments

However, I haven't had time to update it in a long time, hence the missing dependencies.

I propose to do the following to improve the situation regarding Binder:

  • Remove the binder folder in this repo to use the one in binder-environments instead (there's a branch on it per branch here).
  • Update the 5.x, master and spyder-stable dependencies in binder-environments with the latest changes here. Right now, those branches are separate, but you can push directly to them and then check that the Binder image is built as expected by clicking on the launch | binder button of the Readme associated to each branch.
  • Create a subrepo for binder-environments in external-deps and sync it with the corresponding branch here.
  • Change test_dependencies_for_binder_in_sync to do use external-deps/binder-environments/binder/environment.yml instead.
  • Add a section in CONTRIBUTING.md explaining how to add/update a dependency in binder-environments. Perhaps we should start merging from 5.x to master on that repo so that contributors don't have to open two PRs there.
  • Remove the launch | binder button in our Readme and add a new section instead called Run Spyder in your browser (or something similar). That should contain a small explanation of what Binder is and three buttons (you can create custom Binder buttons here):
  • Add a new instruction in RELEASE.md so that our release manager don't forget to update the dependencies of the spyder-stable branch in binder-environments after a release.

This way we'd make sure that binder-environments is always up to date and it could be used reliably by users.

@mrclary, I think this would be a cool project for you to take on. Besides, it'd be great if you could work on it because I'm really busy with other things.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @mrclary! I'm going to merge this because it also fixes an error where the Internal console is hidden at startup on DEV mode, which I use lot when debugging things in Spyder.

@ccordoba12 ccordoba12 merged commit 0198e70 into spyder-ide:5.x Oct 15, 2022
ccordoba12 added a commit that referenced this pull request Oct 15, 2022
@mrclary mrclary deleted the issue-19832-fix-bootstrap branch October 17, 2022 16:02
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.

None yet

2 participants