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

Removal of outdated ppc64le arch Dockerfile #1290

Merged
merged 2 commits into from
May 22, 2021

Conversation

consideRatio
Copy link
Collaborator

@consideRatio consideRatio commented May 6, 2021

This PR reverts the addition of the way we at one point supported manual builds of a ppc64le architecture version of the Dockerfile's in this repo.

The key motivation for removing these are that these resources haven't been easy to keep maintained alongside the other files. By removing them, we can reduce maintenance burden and potentially enable a new solution to support building multiple architectures that is from a maintenance standpoint more sustainable.

Copy link
Member

@mathbunnyru mathbunnyru left a comment

Choose a reason for hiding this comment

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

exclude: Dockerfile.ppc64le|Dockerfile.ppc64le.patch

Please, remove this line as well.

@mathbunnyru
Copy link
Member

We need to be sure, that these files are not used by anyone else.
At least, they are not built right now by us in the CI, which is a good thing (for removal).

@mathbunnyru
Copy link
Member

The maintainer @ilsiyar-gaynutdinov-ibm hasn't been maintaining this since Dec 8, 2016 (this is his last commit).
There were some changes though by other people.

This is the original addition:
#305

@consideRatio
Copy link
Collaborator Author

We need to be sure, that these files are not used by anyone else.

I think this is more stringent than is reasonable to aim for, we just can't be sure. But, let's consider the content of the files.

This is from the Dockerfile.ppc64le.patch file removed.

+    'notebook==5.2.*' \
+    'jupyterhub==0.7.*' \
+    'jupyterlab==0.18.*'

This is from the Dockerfile.ppc64le

    'notebook=6.1.3' \
    'jupyterhub=1.1.0' \
    'jupyterlab=2.2.5' && \

This is from the main Dockerfile

    'notebook=6.3.0' \
    'jupyterhub=1.4.0' \
    'jupyterlab=3.0.14' && \

History of changes

I've looked at the git blame information of Dockerfile.ppc64le. Besides #1088 which was made a year ago, this file hasn't been updated for several years and it was non-functional at the time #1088 was created.

I suggest we settle for a heads up notification to the contributors to this file that we are cleaning up the repos logic in preparation to attempt implement a solution of providing additional architecture builds of the dockerfiles that is sustainable to maintain.

@mathbunnyru
Copy link
Member

Thanks for this investigation @consideRatio, it makes me much more sure we can (almost) safely delete these file 👍

My opinion:
I do believe that having code like this that is not built/tested, uses patching, and not pushed to the official docker hub is not a good way to go if we want to maintain the project and I'm ok with deleting these files.

At the same time I do believe that we take some responsibility to try our best to reimplement multi-arch images.
I won't be able to write much of code myself (honestly, because, I'm not that interested in the arm images), but I will do PR reviews / try to help where I can and so on.

I want to hear from @romainx and @parente how they feel about this.

@mathbunnyru
Copy link
Member

And, to be honest, I wanted to send a PR exactly like this myself several days ago, but didn't have much time 😆

@consideRatio
Copy link
Collaborator Author

Pinging @ilsiyar-gaynutdinov-ibm and @oskrdt who have contributed specifically to the ppc64le dockerfile in the past.

@oskrdt
Copy link
Contributor

oskrdt commented May 11, 2021

I updated and tested the base-notebook/Dockerfile.ppc64le file a year ago when I had to make it work for Power architecture, and then pushed to DockerHub to oskrdt/jupyter-base-notebook. At least at that point I can confirm it was working... after that, I haven't done any update. It was useful for me at that time, but I'm not actively working on updating to the latest versions, so I understand the motivation to remove it.
In the other hand, I didn't work with base-notebook/Dockerfile.ppc64le.patch, so I believe it might be broke.
I would like to keep the ppc64le Dockerfile version for a open collaboration team, but we can keep the support internally in case you decide to remove it.
PD. Having multiple architecture support would be awesome.

@consideRatio
Copy link
Collaborator Author

Thanks for the followup @oskrdt!

@consideRatio consideRatio requested a review from romainx May 19, 2021 01:44
@consideRatio
Copy link
Collaborator Author

Friendly ping to @romainx, what do you think?

@consideRatio consideRatio added the type:Maintenance A proposed enhancement to how we maintain this project label May 19, 2021
@consideRatio consideRatio force-pushed the pr/cleanup-outdated-arch-builds branch from 62b3803 to eabfdfe Compare May 21, 2021 12:45
@romainx
Copy link
Collaborator

romainx commented May 21, 2021

@consideRatio and @mathbunnyru sorry for the late answer.

My opinion

  • This Dockerfile (and everything related) is obsolete and not maintained for a while
  • This feature is undocumented (as far as I know)
  • If someone needs it, it could be restored from the history
  • We will shortly replace it with a great multi-arch image 😄

So let's go for the big cleanup! 🚀

Copy link
Collaborator

@romainx romainx left a comment

Choose a reason for hiding this comment

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

See my comment.

@consideRatio
Copy link
Collaborator Author

Wiee thanks for input @romainx!

Go for merge?

@romainx romainx merged commit ed559b6 into jupyter:master May 22, 2021
@romainx
Copy link
Collaborator

romainx commented May 22, 2021

Merged thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:Maintenance A proposed enhancement to how we maintain this project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants