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

pachi 12.70 dcnn #123060

Closed
wants to merge 1 commit into from
Closed

pachi 12.70 dcnn #123060

wants to merge 1 commit into from

Conversation

lemonsqueeze
Copy link

Hi,

I'm Pachi's current maintainer.
Following up on pachi recent build issue, i took at the pachi.rb formula and found it could use an update : currently it does a nodcnn build which is really unfortunate, and downloads deprecated data files which are no longer used.
See original comment for details.

@chenrui333 suggested i open a PR to try a new build, so here it is.

I don't have access to a mac, but hopefully should work out with CI testing ...
This is my first PR to homebrew-core so if anything's out of place please let me know.

@BrewTestBot BrewTestBot added the deprecated license Formula uses a deprecated SPDX license which should be updated label Feb 12, 2023
@chenrui333
Copy link
Member

Thanks @lemonsqueeze, there are some style issues which can be fixed with brew style --fix pachi

@chenrui333 chenrui333 mentioned this pull request Feb 12, 2023
depends_on "caffe"

resource "datafiles" do
url "https://launchpad.net/~lemonsqueeze/+archive/ubuntu/pachi/+sourcefiles/pachi-go-data/12.70/pachi-go-data_12.70.orig.tar.xz"
Copy link
Member

Choose a reason for hiding this comment

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

why not host this file in the github release?

Copy link
Author

Choose a reason for hiding this comment

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

The dcnn files (detlef54.*) are too big to keep in git, that's why they are missing from the github source release. They are included in the binary releases (windows, linux-static) though, so could get them from github instead of launchpad if that's an issue. The good thing with this launchpad resource is it's got just the datafiles, the bad : it usually takes me a day or two to update it after a release. So could pull everything from github if that's preferrable.

Copy link
Member

Choose a reason for hiding this comment

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

So where are they kept inbetween binary releases?

@chenrui333
Copy link
Member

we have actually deprecate caffe

  pachi:
    * Dependency 'caffe' is deprecated but has un-deprecated dependents. Either
      un-deprecate 'caffe' or deprecate it and all of its dependents.

rewrote recipe to build Pachi with dcnn support, since homebrew already builds caffe.
(Pachi with dcnn support has a lot more value)

take advantage of DESTDIR and DATADIR build variables so we can just install with "make install".
removed old caveat notice about datafiles having to be in current directory, no longer the case.

no longer downloads old patterns files (not used since Pachi 12.10) and opening book from external site.
get missing dcnn datafiles from launchpad (could get them from Pachi binary release too).
@lemonsqueeze
Copy link
Author

  pachi:
    * Dependency 'caffe' is deprecated but has un-deprecated dependents. Either
      un-deprecate 'caffe' or deprecate it and all of its dependents.

Yeah, this is unfortunate. Is there anything i can do about it ?
Or should i ask caffe to be un-deprecated ?

@lemonsqueeze
Copy link
Author

Fixed style issues, looks like it passes now.

@chenrui333
Copy link
Member

  pachi:
    * Dependency 'caffe' is deprecated but has un-deprecated dependents. Either
      un-deprecate 'caffe' or deprecate it and all of its dependents.

Yeah, this is unfortunate. Is there anything i can do about it ? Or should i ask caffe to be un-deprecated ?

see discussions in this thread, #119433

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Thanks for a PR. A few comments.

url "https://sainet-dist.s3.amazonaws.com/pachi_patterns.zip"
sha256 "73045eed2a15c5cb54bcdb7e60b106729009fa0a809d388dfd80f26c07ca7cbc"
end
depends_on "caffe"
Copy link
Member

Choose a reason for hiding this comment

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

Is caffe required? We no longer allow caffe to be used as a dependency because it is deprecated.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, currently Pachi depends on caffe to evaluate its neural network. It's possible to do a nodcnn build, in which case caffe is not needed, but really unfortunate for users (Pachi loses much of its value without dcnn).

Copy link
Member

Choose a reason for hiding this comment

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

I think that's the trade-off we'll have to make here. We can use dcnn once it works with software that is not deprecated.

end

def install
ENV["DESTDIR"] = HOMEBREW_PREFIX
Copy link
Member

Choose a reason for hiding this comment

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

We don't actually want to install directly into HOMEBREW_PREFIX, I think.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, was not sure about this one. Maybe i should set PREFIX = HOMEBEW_PREFIX instead, this is what i intended to do.

@lemonsqueeze
Copy link
Author

I took a look at #119433. Homebrew is not alone in dropping caffe : it's not in debian testing anymore and already it's gone from ubuntu 22.04, so i have to build it in my ppa. To be honest i don't really understand why everybody drops it. Software engineering is a strange place sometimes, in the real world nobody would think of tearing down a house that's just finished building just because the builders have gone in maintenance mode and there are other taller, shinier buildings being built. The house is built, it's stable, mature and useful, might as well use it !

In this case it provides an efficient way to evaluate a neural network in production code while keeping code footprint small. I don't want to pull in tensorflow, pytorch or another huge framework just for that, it would be ridiculous. If someone wants to recommend another library that does that i'll consider migrating. Otherwise if caffe gets dropped completely i guess i'll have to include a caffe build in Pachi, which is a bit ridiculous when there's already a working caffe formula ...

I'm curious, from Homebrew's point of view does it really hurt that much to keep caffe around ?
It's not like it's so broken that it doesn't build anymore or something like that, is it ?

end

def install
ENV["DESTDIR"] = HOMEBREW_PREFIX
ENV["DATADIR"] = pkgshare
ENV["GENERIC"] = "1" # make cpu-generic binary (intel)
Copy link
Member

Choose a reason for hiding this comment

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

So what happens on ARM?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, should be intel only. I guess i should put this on a switch.

@lemonsqueeze
Copy link
Author

As it is this formula can't succeed (caffe deprecated).
I'll see about including a caffe build in Pachi and open a new PR, i guess that's the only way it could build in homebrew.
Thanks for all the useful feedback.

@SMillerDev
Copy link
Member

I'll see about including a caffe build in Pachi and open a new PR, i guess that's the only way it could build in homebrew.

No, that would still be disallowed. We don't want to vendor deprecated software. The only way to build it in Homebrew is to switch away from deprecated dependencies.

@lemonsqueeze
Copy link
Author

Closing per above comment.

@SMillerDev
Copy link
Member

To be honest i don't really understand why everybody drops it. Software engineering is a strange place sometimes, in the real world nobody would think of tearing down a house that's just finished building just because the builders have gone in maintenance mode and there are other taller, shinier buildings being built. The house is built, it's stable, mature and useful, might as well use it !

But people redo the paint with new paint, replace the windowsills and remove asbestos to make sure the house is still safe to live in. If the builders promise you to maintain the house and then just disappear for a long time you'd be worried about the state of your house too.

@lemonsqueeze lemonsqueeze mentioned this pull request Feb 15, 2023
@lemonsqueeze
Copy link
Author

No, that would still be disallowed. We don't want to vendor deprecated software. The only way to build it in Homebrew is to switch away from deprecated dependencies.

Looks like you misunderstood what i wrote. There wouldn't be any deprecated dependency then, Pachi would depend on Openblas, Boost, Protobuf, Glog, Gflags and the Standard C++ Library. Are any of these deprecated as well ?

While i certainly respect Homebrew's decision to not package Caffe anymore, individual projects remain free to make whatever technical choices best suit their needs. Homebrew should certainly respect that, and it is none of its business what these choices even are are as long as the software builds and functions properly. A comment like this where someone tries to force his political decisions down a project's throat is concerning to say the least. I may be wrong in assuming that your views reflect that of Homebrew's of course.

@SMillerDev
Copy link
Member

There wouldn't be any deprecated dependency then, Pachi would depend on Openblas, Boost, Protobuf, Glog, Gflags and the Standard C++ Library. Are any of these deprecated as well ?

It will include a version of Caffe then right? Because that would be vendoring a deprecated dependency.

individual projects remain free to make whatever technical choices best suit their needs.

They certainly are and so is Homebrew when it decides what to package.

force his political decisions down a project's throat

Nobody is forcing any project to do something. I'm just outlining the requirements for software to be included in Homebrew.

@lemonsqueeze
Copy link
Author

It will include a version of Caffe then right? Because that would be vendoring a deprecated dependency.

Like i said, Pachi would have its own neural network code and link against Boost and Openblas. It wouldn't ship any Caffe library binary, nor would there be any trace of one on the system. So as far as homebrew is concerned once you're done deprecating Caffe it's effectively gone from the system. Isn't it all that matters ?

@lemonsqueeze
Copy link
Author

Well, I guess we'll leave it at that.
First encounter with homebrew police, i must say i'm impressed.
Maybe it'd be good to put badges on the avatars, that would make things clear.
For some time i thought i was talking to another software engineer, hence the complete misunderstanding.

@github-actions github-actions bot added the outdated PR was locked due to age label Mar 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
deprecated license Formula uses a deprecated SPDX license which should be updated outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants