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

Update DSCI 100 R and Python Docker Images #38

Merged
merged 6 commits into from
Jan 3, 2024
Merged

Update DSCI 100 R and Python Docker Images #38

merged 6 commits into from
Jan 3, 2024

Conversation

briank-git
Copy link
Contributor

@briank-git briank-git commented Jan 2, 2024

Copy link
Contributor

@trevorcampbell trevorcampbell left a comment

Choose a reason for hiding this comment

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

@briank-git thanks for taking care of this so quickly. A few questions/comments in this review.

Also I assume that all the other upgrades will just follow by rebuilding the dockerfile (which will pull in latest versions of everything) ?

@@ -0,0 +1,2 @@
[pull]
rebase = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we change the pull strategy? I wouldn't normally go for rebase -- any particular reason?

Copy link
Contributor Author

@briank-git briank-git Jan 3, 2024

Choose a reason for hiding this comment

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

I can't find the original slack thread where it was suggested, but no, there's no particular reason. We can use any other strategy. Maybe ff only?

It's just to suppress this error which prompts students to choose a pull strategy (have to use terminal):

Hint: You have divergent branches and need to specify how to reconcile them.
Hint: You can do so by running one of the following commands sometime before
Hint: your next pull:
Hint: 
Hint:   git config pull.rebase false  # merge
Hint:   git config pull.rebase true   # rebase
Hint:   git config pull.ff only       # fast-forward only

Copy link
Contributor

@trevorcampbell trevorcampbell Jan 3, 2024

Choose a reason for hiding this comment

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

Ah I see! It's not for this repo, it's for copying into the docker image later on to avoid students getting confusing error messages. Let's use the default strategy which is pull.rebase false (so that what they see in class is the common default in the real world)

# update scikit-learn to the current @main (July 20, 2023)
# to get this merged PR: https://github.com/scikit-learn/scikit-learn/pull/26772
# which fixes this issue: https://github.com/scikit-learn/scikit-learn/issues/26768
RUN pip install --upgrade git+https://github.com/scikit-learn/scikit-learn@7de59b2017f39048880a3858d32c8caef9308954
Copy link
Contributor

Choose a reason for hiding this comment

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

What installs scikit-learn now that this is gone?

Copy link
Contributor

Choose a reason for hiding this comment

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

(and does the latest release include the two fixes listed in the comments?)

Copy link
Contributor Author

@briank-git briank-git Jan 3, 2024

Choose a reason for hiding this comment

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

scikit-learn 1.3.1 is installed by the base image jupyter/scipy-notebook. Here's a link to the relevant patch notes for 1.3.1, fixed by PR #26772.

COPY jupyter_server_config.py /home/${NB_USER}/.jupyter

# Copy gitconfig that sets global default pull strategy to rebase
COPY .gitconfig /home/${NB_USER}/
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor

Choose a reason for hiding this comment

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

@briank-git please edit the commented out line on 48 above to reflect the default merge strat

@@ -0,0 +1,2 @@
[pull]
rebase = true
Copy link
Contributor

Choose a reason for hiding this comment

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

same Q -- why changing the repo default to rebase?

Copy link
Contributor

Choose a reason for hiding this comment

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

Handled by above discussion

@briank-git
Copy link
Contributor Author

@briank-git thanks for taking care of this so quickly. A few questions/comments in this review.

Also I assume that all the other upgrades will just follow by rebuilding the dockerfile (which will pull in latest versions of everything) ?

Yes, I did test builds yesterday and new versions of RISE, jupyterlab-git, pexpect were installed.

@trevorcampbell trevorcampbell merged commit 9b29d51 into UBC-DSCI:main Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants