Tweaks to improve cleanliness of code; removes warnings that appear in Eclipse #11
Conversation
warnings when it comes to using incubator features, and covers "Scanner in" in a try block.
.project
Outdated
There was a problem hiding this comment.
Please do not include any project files, having the whole project as a single file at the root is deliberate.
bin/.gitignore
Outdated
There was a problem hiding this comment.
Please remove, there's already a .gitignore at the root
src/com/llama4j/Llama3.java
Outdated
| } | ||
| } | ||
| try (Scanner in = new Scanner(System.in)) { | ||
| while (true) { |
There was a problem hiding this comment.
The indentation here is too high, please follow the same as the rest of the file (4 spaces).
There was a problem hiding this comment.
It looks a bit long in this snippet, but I'm looking at the code right now and the indentation seems to be fine. It was formatted this way by my IDE automatically. You may need to tweak this on your end to get whatever specific formatting you'd prefer, since I tried the two spaces thing and it made the code look very strange IMO.
There was a problem hiding this comment.
You are using tabs instead of spaces, GitHub render tabs as 8 spaces. Your IDE should have a setting to set it. Please keep 4 spaces as the rest of the file.
src/com/llama4j/Llama3.java
Outdated
| @@ -346,7 +347,8 @@ public int byteSize() { | |||
| } | |||
| } | |||
|
|
|||
| private void loadModelImpl(FileChannel fileChannel) throws IOException { | |||
| @SuppressWarnings("preview") | |||
| private void loadModelImpl(FileChannel fileChannel) throws IOException { | |||
There was a problem hiding this comment.
Same as above. I'm just using the default formatting and it appears to work on my end- I'm just matching my formatting to the rest of the documents.
src/com/llama4j/Llama3.java
Outdated
| @@ -726,7 +728,7 @@ private static Tokenizer createTokenizer(Map<String, Object> metadata, Vocabular | |||
|
|
|||
| int allTokens = vocabulary.size(); | |||
| int baseTokens = 128000; // assume all tokens after the base ones are special. | |||
| int reservedSpecialTokens = allTokens - baseTokens; | |||
| //int reservedSpecialTokens = allTokens - baseTokens; | |||
There was a problem hiding this comment.
This is indeed unused, do not comment it out, remove instead.
src/com/llama4j/Llama3.java
Outdated
There was a problem hiding this comment.
Do not move the Llama3.java file outside the root. I understand that this is the way IDEs work, but the project is meant to be cloned and executed with the minimum friction possible.
src/com/llama4j/Llama3.java
Outdated
| float wavelen = (float) (2.0 * Math.PI / freq); | ||
| if (wavelen < hiFreqWavelen) { | ||
|
|
There was a problem hiding this comment.
I think this changes the original behavior. We must keep sync with the RoPE scaling added in Llama 3.1
Here's the reference (RoPE scaling section):
There was a problem hiding this comment.
I'm not arguing that, it's just that you're doing this after that check:
freq = freq
This doesn't do anything, technically. You basically comment it out with no change to functionality. That said, I agree that having it there (even if commented out) is crucial to understanding the code. I'll reset the code to how you had it though.
I'm curious- what program are you writing this in?
There was a problem hiding this comment.
I ported it from the Python snippet, you can refactor it into something equivalent e.g. substitute the freq = freq; by a comment stating that no modification is done in that case, although you may get a warning about an empty code block.
It must remain functionally equivalent to the original (this is a hard constraint); and ideally close to the original as well, so it can be easily understood and compared by others.
I'm curious- what program are you writing this in?
I use IntelliJ and casually (neo)vim and run it from the command-line via jbang which supports a very convenient --debug flag.
|
Alright, sorry. This was the first time I'd ever done a pull request since I usually just build my own tools. I'll fix it up when I have time. |
|
Alright, I've adjusted it based on your feedback. That said, this version does include a git ignore file, but that's specifically so it won't push the classpath, project file, and other unwanted items. I think you can delete that on your end though. Sorry for all of the mistakes! I hope this is useful to you. |
-Surrounded Scanner in runInteractive() in try blocks
-Tweaked precomputeFreqsCis() and removed code that doesn't appear to do anything (triggers a warning in Eclipse)
-Added @SuppressWarnings("preview") to loadModelImpl()