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.

Sign up for GitHub

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

Jump to bottom

[Feature Request] Infer NDBuffer Rank From DimList #4

Open
lsh opened this issue May 3, 2023 · 3 comments
Open

[Feature Request] Infer NDBuffer Rank From DimList #4

lsh opened this issue May 3, 2023 · 3 comments
Labels
enhancement New feature or request mojo Issues that are related to mojo mojo-lang Tag for all issues related to language. mojo-repo Tag all issues with this label

Comments

@lsh
Copy link
Contributor

lsh commented May 3, 2023

In most APIs, the rank of a multidimensional array would be assumed to be equivalent to the shape.

I noticed that the invocation for NDBuffer is NDBuffer[rank, shape, dtype](). It would be ergonomic to have a constructor that assumes rank from shape, so it could just be NDBuffer[DimList(2,3), DType.ui32](),
or even better NDBuffer[(2,3), DType.ui32]()
@Mogball
Copy link
Collaborator

Mogball commented May 4, 2023

Thanks for the issue!

This is a current limitation of how we handle dependent parameters, and there are other types and functions that suffer from this. The key issue is that the shape parameter depends on rank, and parameters are resolved left-to-right, so rank has to be defined first. That said, I think this is something we should support, since it is natural and intuitive to infer rank from shape.

@Mogball Mogball added the enhancement New feature or request label May 4, 2023
@lattner
Copy link
Collaborator

lattner commented May 7, 2023 •
edited
Loading

@abduld this seems like we can just directly improve NDBuffer. Could we change it from:

struct NDBuffer[
    rank: Int,
    shape: DimList,
    type: DType,
]:

to:

struct NDBuffer[
    shape: DimList,
    type: DType,
]:
  alias rank = len(shape) 
  ...

DimList doesn't currently have a __len__ method, but its underlying storage type does.

@abduld
Copy link
Contributor

abduld commented May 7, 2023

Yes, we can certainly improve (and will improve) the API for NDBuffer. Thanks @lsh for the suggestion

@abduld abduld self-assigned this May 7, 2023
@ematejska ematejska added the mojo Issues that are related to mojo label Sep 7, 2023
@jangwonpark74 jangwonpark74 mentioned this issue Sep 9, 2023
Closed
@ayunillariy ayunillariy mentioned this issue Sep 12, 2023
Closed
@DmarshalTU DmarshalTU mentioned this issue Sep 12, 2023
Closed
1 task
@abtzz abtzz mentioned this issue Oct 9, 2023
Closed
@sangshuduo sangshuduo mentioned this issue Oct 14, 2023
Closed
@corvinux corvinux mentioned this issue Oct 23, 2023
Closed
Closed
@danny-su danny-su mentioned this issue Dec 25, 2023
Closed
@1-hussy 1-hussy mentioned this issue Jan 5, 2024
Open
@dwhire dwhire mentioned this issue Jan 11, 2024
Closed
@betapro betapro mentioned this issue Jan 26, 2024
Closed
@Mogball Mogball added the mojo-lang Tag for all issues related to language. label Feb 22, 2024
@abdulghani91 abdulghani91 mentioned this issue Feb 26, 2024
Closed
@Will-Zhao0 Will-Zhao0 mentioned this issue Feb 29, 2024
Closed
@sneekyfoxx sneekyfoxx mentioned this issue Mar 4, 2024
Closed
@davidxiaozhi davidxiaozhi mentioned this issue Mar 6, 2024
Open
JoeLoser pushed a commit to JoeLoser/mojo that referenced this issue Mar 28, 2024
@abduld
[Stdlib] Further remove pop operations from the dtype checks, NFC (#3…
bf78b6b 
…2290)

This continues to remove the pop operations from the dtype checks
without getting recursive elaborator errors

E.g

```
no work left, no deferred search, and no recursion?
UNREACHABLE executed at /Users/abduld/code/modular/KGEN/lib/Elaborator/Elaborator.cpp:2703!
PLEASE submit a bug report to [Internal Link] and include the crash backtrace.
 #0 0x0000000102c4ca08 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/Users/abduld/code/modular/.derived/build-release/bin/mojo-prime-package-cache 0x1000c4a08)
 #1 0x0000000102c4ab68 llvm::sys::RunSignalHandlers() (/Users/abduld/code/modular/.derived/build-release/bin/mojo-prime-package-cache 0x1000c2b68)
 modularml#2 0x0000000102c4d0a8 SignalHandler(int) (/Users/abduld/code/modular/.derived/build-release/bin/mojo-prime-package-cache 0x1000c50a8)
 modularml#3 0x00000001880e02a4 (/usr/lib/system/libsystem_platform.dylib 0x1803fc2a4)
 modularml#4 0x00000001880b1cec (/usr/lib/system/libsystem_pthread.dylib 0x1803cdcec)
 modularml#5 0x0000000187feb2c8 (/usr/lib/system/libsystem_c.dylib 0x1803072c8)
 modularml#6 0x0000000102be03a0 llvm::install_out_of_memory_new_handler() (/Users/abduld/code/modular/.derived/build-release/bin/mojo-prime-package-cache 0x1000583a0)
```

modular-orig-commit: ebb7c424801291198503fde0ebdd6fd28da8f2e9
@ematejska ematejska added the mojo-repo Tag all issues with this label label Apr 29, 2024
modularbot pushed a commit that referenced this issue May 29, 2024
@Mogball @modularbot
Revert "[External] [stdlib] Support Dict.popitem() (#40688)"
b9453e9 
This PR introduces nondeterminism into the testsuite. `test_dict.mojo`
nondeterministically fails with

```
[M] ➜  modular git:(1853f9d3e9) mojo /Users/jeff/Documents/modular/******/test/stdlib/collections/test_dict.mojo
Test test_basic ...PASS
Test test_multiple_resizes ...PASS
Test test_big_dict ...PASS
Test test_compact ...PASS
Test test_compact_with_elements ...PASS
Test test_pop_default ...PASS
Test test_key_error ...PASS
Test test_iter ...PASS
Test test_iter_keys ...PASS
Test test_iter_values ...PASS
Test test_iter_values_mut ...PASS
Test test_iter_items ...PASS
Test test_dict_copy ...PASS
Test test_dict_copy_add_new_item ...PASS
Test test_dict_copy_delete_original ...PASS
Test test_dict_copy_calls_copy_constructor ...PASS
Test test_dict_update_nominal ...PASS
Test test_dict_update_empty_origin ...PASS
Test test_dict_update_empty_new ...PASS
Test test_mojo_issue_1729 ...PASS
Test test dict or ...PASS
Test test dict popteim ...get: wrong variant type
Please submit a bug report to https://github.com/modularml/mojo/issues and include the crash backtrace along with all the relevant source codes.
Stack dump:
0.      Program arguments: mojo /Users/jeff/Documents/modular/******/test/stdlib/collections/test_dict.mojo
 #0 0x00000001043a10b0 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo 0x1000c90b0)
 #1 0x000000010439f210 llvm::sys::RunSignalHandlers() (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo 0x1000c7210)
 #2 0x00000001043a1750 SignalHandler(int) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo 0x1000c9750)
 #3 0x00000001ab1b2a24 (/usr/lib/system/libsystem_platform.dylib 0x18042ea24)
 #4 0xffff8002a81b8510
 #5 0x00000001047c1608 M::KGEN::ExecutionEngine::runProgram(llvm::StringRef, llvm::StringRef, llvm::function_ref<M::ErrorOrSuccess (void*)>) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo 0x1004e9608)
 #6 0x00000001042f8270 executeMain(mlir::ModuleOp, mlir::SymbolTable const&, M::KGEN::ExecutionEngine*, M::LLCL::Runtime&, llvm::ArrayRef<char const*>) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo 0x100020270)
 #7 0x00000001042f7cb8 run(M::State const&) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo 0x10001fcb8)
 #8 0x00000001042df774 main (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo 0x100007774)
 #9 0x00000001aae2bf28
[1]    44318 trace trap  mojo
```

MODULAR_ORIG_COMMIT_REV_ID: ee1c665669902106df680fe6c6d2599897665ff5
modularbot pushed a commit that referenced this issue Jun 7, 2024
@Mogball @modularbot
Revert "[External] [stdlib] Support Dict.popitem() (#40688)"
c0fd441 
This PR introduces nondeterminism into the testsuite. `test_dict.mojo`
nondeterministically fails with

```
[M] ➜  modular git:(1853f9d3e9) mojo /Users/jeff/Documents/modular/******/test/stdlib/collections/test_dict.mojo
Test test_basic ...PASS
Test test_multiple_resizes ...PASS
Test test_big_dict ...PASS
Test test_compact ...PASS
Test test_compact_with_elements ...PASS
Test test_pop_default ...PASS
Test test_key_error ...PASS
Test test_iter ...PASS
Test test_iter_keys ...PASS
Test test_iter_values ...PASS
Test test_iter_values_mut ...PASS
Test test_iter_items ...PASS
Test test_dict_copy ...PASS
Test test_dict_copy_add_new_item ...PASS
Test test_dict_copy_delete_original ...PASS
Test test_dict_copy_calls_copy_constructor ...PASS
Test test_dict_update_nominal ...PASS
Test test_dict_update_empty_origin ...PASS
Test test_dict_update_empty_new ...PASS
Test test_mojo_issue_1729 ...PASS
Test test dict or ...PASS
Test test dict popteim ...get: wrong variant type
Please submit a bug report to https://github.com/modularml/mojo/issues and include the crash backtrace along with all the relevant source codes.
Stack dump:
0.      Program arguments: mojo /Users/jeff/Documents/modular/******/test/stdlib/collections/test_dict.mojo
 #0 0x00000001043a10b0 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo 0x1000c90b0)
 #1 0x000000010439f210 llvm::sys::RunSignalHandlers() (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo 0x1000c7210)
 #2 0x00000001043a1750 SignalHandler(int) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo 0x1000c9750)
 #3 0x00000001ab1b2a24 (/usr/lib/system/libsystem_platform.dylib 0x18042ea24)
 #4 0xffff8002a81b8510
 #5 0x00000001047c1608 M::KGEN::ExecutionEngine::runProgram(llvm::StringRef, llvm::StringRef, llvm::function_ref<M::ErrorOrSuccess (void*)>) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo 0x1004e9608)
 #6 0x00000001042f8270 executeMain(mlir::ModuleOp, mlir::SymbolTable const&, M::KGEN::ExecutionEngine*, M::LLCL::Runtime&, llvm::ArrayRef<char const*>) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo 0x100020270)
 #7 0x00000001042f7cb8 run(M::State const&) (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo 0x10001fcb8)
 #8 0x00000001042df774 main (/Users/jeff/Documents/modular/.derived/build-relwithdebinfo/bin/mojo 0x100007774)
 #9 0x00000001aae2bf28
[1]    44318 trace trap  mojo
```

MODULAR_ORIG_COMMIT_REV_ID: ee1c665669902106df680fe6c6d2599897665ff5
@JamesClarke7283 JamesClarke7283 mentioned this issue Jun 22, 2024
Open
@Karolcha100 Karolcha100 mentioned this issue Jul 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request mojo Issues that are related to mojo mojo-lang Tag for all issues related to language. mojo-repo Tag all issues with this label
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@abduld @lsh @Mogball @lattner @ematejska @goldiegadde