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

Use different trigger properties for avx2/avx512 extension profiles? #6933

Closed
treo opened this issue Jan 4, 2019 · 11 comments
Closed

Use different trigger properties for avx2/avx512 extension profiles? #6933

treo opened this issue Jan 4, 2019 · 11 comments

Comments

@treo
Copy link
Member

treo commented Jan 4, 2019

While trying to figure out #6932 I've come across this:

<activation>
<property>
<name>javacpp.extension</name>
<value>avx2</value>
</property>
</activation>
<properties>
<javacpp.platform.extension>-avx2</javacpp.platform.extension>
</properties>

So we are using javacpp.extension here to trigger javacpp.platform.extension. This resulted in a bit of confusion on my side. Since we have to set libnd4j.extension anyway for this case, why not trigger the profile on it and skip using the javacpp namespace for that property?

/cc @saudet

@saudet
Copy link
Contributor

saudet commented Jan 4, 2019

I probably meant to set libnd4j.extension to javacpp.extension here:
https://github.com/deeplearning4j/deeplearning4j/blob/master/libnd4j/pom.xml#L69
We'd just need to set javacpp.extension and both would get set.

@treo
Copy link
Member Author

treo commented Jan 4, 2019

My biggest problem with using the name javacpp.extension is that it insinuates that we are using a javacpp feature directly. When I came across it first, I started looking for documentation for javacpp.

Also the line that you've linked would result in a slightly incompatible way of setting extensions, if I got that correctly.

When building libnd4j we set -Dlibnd4j.extension=avx2, but when using javacpp.platform.extension it is usually set as -Djavacpp.platform.extension=-avx2.

@saudet
Copy link
Contributor

saudet commented Jan 4, 2019

And Java typically includes the ., - or whatever in the prefixes or suffixes, just continuing the tradition here :)

@treo
Copy link
Member Author

treo commented Jan 4, 2019

It is just that there are several very similarly named things that do just slightly different stuff accepting just slightly different options which aren't documented and should in my opinion not even exist as separate things

@saudet
Copy link
Contributor

saudet commented Jan 4, 2019

Sure, feel free to change this however you like. The way it is right now is close enough to how the presets work, but if you wish to maintain Deeplearning4j yourself in a different way, go for it!

@treo
Copy link
Member Author

treo commented Jan 4, 2019

As far as I can tell none of the presets are using a javacpp.extension property, instead they use javacpp.platform.extension.

If possible I'd love to have just javacpp.platform.extension and do away with all other methods of setting the extension.

@sshepel
Copy link
Contributor

sshepel commented Jan 4, 2019

As an option maybe we can unify these two pros to just platform.extension?
And underneaths set appropriate properties for specific module...

@saudet
Copy link
Contributor

saudet commented Jan 5, 2019

@sshepel That's what it's already doing now, but @treo wants to remove that.

@treo We could do that, making sure the build script accepts an extension both with and without hyphen. I think it already does though.

@sshepel
Copy link
Contributor

sshepel commented Jan 5, 2019

@saudet I think you misunderstood @treo comments...
ATM we need to provide both javacpp.extension and libnd4j.extension to build nd4j-native with specific extension... which is a bit confusing for end users... and as it turned out not documented well...

How about to move this conversation to internal chat...

treo added a commit that referenced this issue Jan 7, 2019
This drops javacpp.extension in favor of only using libnd4j.extension
to trigger the proper profiles that will set all other required properties.

So far only two possible values are supported: avx2 and avx512.

Fixes #6933
@lock
Copy link

lock bot commented Feb 8, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Feb 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants