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

Added gemmlowp package #3240

Merged
merged 1 commit into from Mar 7, 2017
Merged

Added gemmlowp package #3240

merged 1 commit into from Mar 7, 2017

Conversation

muakasan
Copy link
Contributor

No description provided.

"""Google low-precision matrix multiplication library"""

homepage = "https://github.com/google/gemmlowp"
version('a6f29d9ac', git='https://github.com/google/gemmlowp.git',
Copy link
Member

@davydden davydden Feb 26, 2017

Choose a reason for hiding this comment

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

Cal it develop. Same with another package without releases

Copy link
Member

Choose a reason for hiding this comment

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

The problem with calling it develop is, what happens when someone adds a newer commit? You can't also call that develop.

I think we should ask or encourage the developers to make releases. But obviously we have no control over that. For packages that don't make releases and expect all users to use the latest version, they often don't support problems in older commits. We should honor there wishes and simply install the latest develop or master branch, whichever they recommend.

I would also like to hear @citibeth's thoughts. I know there are security concerns about this approach.

Copy link
Member

Choose a reason for hiding this comment

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

I would remove a specific commit here. But I fully agree, there should be some releases, even 0.01

Copy link
Member

Choose a reason for hiding this comment

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

@davydden: we're likely to need more references to specific commits as tensorflow evolves. We only packaged the latest version, so there is only one version here now, but this isn't intended to be a reference to the top of develop. It really is a specific reference.

Copy link
Member

Choose a reason for hiding this comment

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

Obviously though if the google folks ever add versions we'd use them...

@citibeth
Copy link
Member

citibeth commented Feb 26, 2017 via email

@tgamblin
Copy link
Member

tgamblin commented Feb 26, 2017

Hey guys. For the hackathon here, we used specific commit ids because tensorflow wants specific commit ids. It's likely that if we incorporate tensorflow into spack that it will continue to want specific commit ids, and it doesn't seem like Google provides (or will provide soon) versions or releases for these libraries.

So in this case the intent is actually not to make this into a "latest release" branch, like we usually use develop. It's really to refer to a specific commit that tensorflow wants. So I think the commit id is more descriptive. We could think about adding a date or maybe using the TF version that wants this commit, but I'm not sure if it's worth coming up with a version scheme when the main dependent refers directly to the commit, anyway.

@tgamblin tgamblin added the WIP label Feb 26, 2017
Copy link
Member

@citibeth citibeth left a comment

Choose a reason for hiding this comment

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

Please change the version download to this:

    # No official released versions yet
    version('0.0.0.1', git='https://github.com/google/gemmlowp.git',
commit='a6f29d8ac48d63293f845f2253eccbf86bc28321')

Version 0.0.0.1 means "the first Spack release after version 0.0.0", and 0.0.0 means "there are no official releases yet". The advantage of 0.0.0.1 as the version number, rather than using the git hash, is that it will sort. This is the solution we've used in similar cases elsewhere.

@davydden
Copy link
Member

@citibeth good idea

@adamjstewart
Copy link
Member

I'm fine with using git commits as versions if that's how tensorflow works. We lose the ability to sort by version, and we'll need to use preferred=True, but it's better than making our own fake releases. What happens when someone contacts the developer and complains that they can't get 0.0.0.1 to install? The developer would be pretty confused. But if we use the commit hash, it's more descriptive.

@citibeth
Copy link
Member

citibeth commented Feb 27, 2017 via email

@tgamblin tgamblin dismissed citibeth’s stale review March 7, 2017 15:00

Dismissing per decision in #3242

@tgamblin tgamblin merged commit 5d1a168 into spack:develop Mar 7, 2017
diaena pushed a commit to diaena/spack that referenced this pull request May 26, 2017
xavierandrade pushed a commit to xavierandrade/spack that referenced this pull request Jun 16, 2017
EmreAtes pushed a commit to EmreAtes/spack that referenced this pull request Jul 10, 2017
amklinv pushed a commit that referenced this pull request Jul 17, 2017
healther pushed a commit to electronicvisions/spack that referenced this pull request Jul 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants