-
Notifications
You must be signed in to change notification settings - Fork 143
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
Update to 24.2 WIP #89
Conversation
The type parameter for the memcpy function is now automatically inferred. This means that calls to memcpy of the form memcpy[Dtype.xyz](...) will no longer work and the user would have to change the code to memcpy(...).
As per The simd_load(), simd_store(), aligned_simd_load(), and aligned_simd_store() methods on DTypePointer, Buffer, and NDBuffer have been merged into a more expressive set of load() and store() methods with keyword-only width and alignment parameters:
DynamicVector has been renamed to List, and has moved from the collections.vector module to the collections.list module. I
StaticTuple parameter order has changed to StaticTuple[type, size] for consistency with SIMD and similar collection types.
List.push_back() has been removed. Please use the append() function instead.
_steal_ptr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for undertaking this. It is a fairly chunky set of changes for this release.
And I will definitely pull a copy and be able to help figure out any errors later today. Thanks again!
llama2.mojo
Outdated
@@ -379,7 +379,7 @@ struct Config: | |||
var config_data_raw = f.read_bytes(bytes_of_config_params) | |||
f.close() | |||
# correct Tensor type and shape for easy reading, without copying data | |||
var int32_ptr = config_data_raw._steal_ptr().bitcast[DType.int32]() | |||
var int32_ptr = config_data_raw.bitcast[DType.int32]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure we don't still need to steal the pointer also? config_data_raw
is going to be deleted after last use which will free this pointer before we are done with it if it is still the owner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure
With it remaining the error is
/home/anthony/llama2.mojo/llama2.mojo:382:40: error: 'List[SIMD[si8, 1]]' value has no attribute '_steal_ptr'
var int32_ptr = config_data_raw._steal_ptr().bitcast[DType.int32]()
with it removed the error is
/home/anthony/llama2.mojo/llama2.mojo:382:40: error: 'List[SIMD[si8, 1]]' value has no attribute 'bitcast'
var int32_ptr = config_data_raw.bitcast[DType.int32]()
So the theme seems to be 'List[SIMD[si8, 1]]' value has no attribute
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sure. 😃 The correct thing is to steal the pointer so we take ownership. The method signature has just changed so it is a bit more elaborate now.
The full correct line here is:
var int32_ptr = DTypePointer[DType.int8](config_data_raw.steal_ptr().value).bitcast[DType.int32]()
.
It can be split into multiple lines if you think it is clearer but the steps are steal the pointer, convert to DTypePointer, and then bitcast to the int32 representation we want.
llama2.mojo
Outdated
@@ -466,7 +466,7 @@ struct TransformerWeights: | |||
# So we can't reshape to target shape because dims don't match | |||
var tmp = f.read_bytes(shape.num_elements() * sizeof[DType.float32]()) | |||
bytes_read += shape.num_elements() * sizeof[DType.float32]() | |||
var data = tmp._steal_ptr().bitcast[DType.float32]() | |||
var data = tmp.bitcast[DType.float32]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, we don't want tmp
to keep ownership.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am guessing that the bitcast errors may all be similar. I referenced the errors above with it either way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, same as above with changes to types is brackets and the variable name.
llama2.mojo
Outdated
@@ -503,7 +503,7 @@ fn read_file(file_name: String, inout buf: FileBuf) raises: | |||
var data = fd.read() | |||
fd.close() | |||
buf.size = data._buffer.size | |||
buf.data = data._steal_ptr().bitcast[DType.uint8]() | |||
buf.data = data.bitcast[DType.uint8]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, we don't want data
to keep ownership.
I pulled this and tried against Mojo 24.2 but I think maybe you are working against the nightly branch? So these changes are likely to be needed in update: This looks like my own mistake not shifting the VS Code extension to 24.2. Your changes here are indeed for 24.2 so ignore this comment. |
Hi Michael, thanks for all the updates 😎, I'll take a closer look and
check version tonight! 😊
…On Mon, Apr 15, 2024, 7:58 PM Michael Kowalski ***@***.***> wrote:
I pulled this and tried against Mojo 24.2 but I think maybe you are
working against the nightly branch? So these changes are likely to be
needed in 24.4 assuming we get a release later this month that makes
these new Mojo APIs public.
—
Reply to this email directly, view it on GitHub
<#89 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEJ6DJAHEC5CLJLKT2XO3YDY5SHT7AVCNFSM6AAAAABGIKGSHOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJYGE2DEMJTHE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
: error: invalid call to 'rand': missing 1 required positional argument: 'size' var r = rand[DType.float32](1)
expected ']' for parameter list fn load[width=width: Int](self, *indices: Int) -> SIMD[DType.float32, width]:
requires a size and pointer declared create new pointer following https://docs.modular.com/mojo/stdlib/random/random#rand alloc(1) for 1 value?
'None' does not implement the '__bool__' method
Hello! Okay so I think I fixed most of the items mentioned. Sorry about I fixed a new error The only two sets of remaining errors seem to be
anthony-chaudhary@394d31f
Not surer if I'm missing something obvious but it seems like this isn't implemented in mojo the same as python?
Edit: Sounds like version thing isn't a concern but just in case this is the output of |
llama2.mojo
Outdated
@@ -937,12 +939,12 @@ fn print_str(s: PointerString): | |||
if (s[1].to_int() == ord("0")) and (s[2].to_int() == ord("x")): | |||
var d1: Int = s[3].to_int() | |||
var d2: Int = s[4].to_int() | |||
print_no_newline(chr(str2num(d1) * 16 + str2num(d2))) | |||
print(chr(str2num(d1) * 16 + str2num(d2))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since print_no_newline
was removed the correct way to mimic this now is with a second argument end=""
otherwise the default behaviour adds the newline character.
llama2.mojo
Outdated
return | ||
# print all chars till null character | ||
var p: Int = 0 | ||
while s[p].to_int() != 0: | ||
print_no_newline(chr(s[p].to_int())) | ||
print(chr(s[p].to_int())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above, end=""
needs to be a second argument provided to print
This error: /home/anthony/llama2.mojo/llama2.mojo:886:13: error: 'None' is not subscriptable, it does not implement the `__getitem__`/`__setitem__` or `__refitem__` methods
if r[0] < cdf: Is because |
Hi @Anthony-Sarkis , I added a few fixes. Now it llama2 is compiling fine |
Awesome, thanks everyone! 😊
…On Wed, Apr 17, 2024, 1:44 AM Aydyn Tairov ***@***.***> wrote:
Merged #89 <#89> into master.
—
Reply to this email directly, view it on GitHub
<#89 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEJ6DJBD56NAAFTDUW25ANTY5YZAHAVCNFSM6AAAAABGIKGSHOVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJSGUYDIMBVGA4DGNY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Related to #88
Work in progress, still getting these errors. I'm new to Mojo so if anyone can assist: