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

Automatically install latest julia version #2046

Merged
merged 16 commits into from
Dec 4, 2023

Conversation

mathbunnyru
Copy link
Member

@mathbunnyru mathbunnyru commented Dec 1, 2023

Describe your changes

  • Automatically determine the latest stable (but non-LTS) version of Julia
  • Julia 1.9.4 is gonna be installed after this PR

Issue ticket if applicable

Checklist (especially for first-time contributors)

  • I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests
  • I will try not to use force-push to make the review process easier for reviewers
  • I have updated the documentation for significant changes

@mathbunnyru
Copy link
Member Author

mathbunnyru commented Dec 1, 2023

@yuvipanda @benz0li please take a look.
This should get rid of PRs which only update Julia version.

@mathbunnyru
Copy link
Member Author

After this operation, 1,043 kB of additional disk space will be used.
jq is lightweight

@yuvipanda
Copy link
Contributor

This looks good to me, but what do you think about rewriting this script in python instead? That would allow us to not use jq. I love jq too, but I always find complex bash gets out of hand fast.

@mathbunnyru
Copy link
Member Author

This looks good to me, but what do you think about rewriting this script in python instead? That would allow us to not use jq. I love jq too, but I always find complex bash gets out of hand fast.

Thanks, I will rewrite this script in Python. I might need to use plumbum package though, it makes my life much easier when I need to call some utilities from Python.

@mathbunnyru
Copy link
Member Author

@yuvipanda done. If you like the Python implementation, I will get rid of jq and setup-julia.sh.
Kept them for now, so you can compare.

Copy link
Contributor

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

Yay for rewriting shell scripts in python :) I think that's the way to go, especially if we find ourselves parsing JSON.

Can you add even a simple docstring to all the functions? I find that helps tremendously to both clarity when reading as well as in keeping them well scoped during modification.

I understand the desire to use plumbum, but I find it pretty difficult to read and understand, especially because it's overriding array syntax for command execution. One of the big reasons for me for writing things in python over bash is that it makes it much easier for newer contributors to onboard and make contributions, and I think plumbum sort of turns python into a hybrid shell situation that makes it a little difficult. So I do have a strong preference for just using subprocess, especially when there's no piping or complex shell-like things involved. Here, we're only using it to call subprocesses and nothing else (as far as I can tell). So what do you think about dropping plumbum and just using subprocess, with the line being 'are we only calling a process, getting its output (optional)? If so, use subprocess, else can use plumbum'.

images/minimal-notebook/setup-scripts/setup-julia.py Outdated Show resolved Hide resolved
images/minimal-notebook/setup-scripts/setup-julia.py Outdated Show resolved Hide resolved
@mathbunnyru
Copy link
Member Author

@yuvipanda done, please, take a look.

Copy link
Contributor

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

yay this is awesome! There's one missing docstring, but otherwise this looks good to go.

Pleasure working with you, @mathbunnyru

@mathbunnyru mathbunnyru merged commit b610485 into jupyter:main Dec 4, 2023
64 checks passed
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.

2 participants