Demonstration: table support for HDF5 #14551
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
daviditen
reviewed
Dec 5, 2019
There was a problem hiding this comment.
Overall I think this looks great! It would be good to include chpldoc comments on each main function and on the H5MetaTable type describing what they are and how to use them so that this will be clearly described in the online documentation.
In the places where you re-declared extern functions from C_HDF5 I think you could have instead passed a pointer to the first array element because the versions in C_HDF5 are expecting e.g. c_ptr(c_string). So it would look like H5TBmake_table(..., meta.names[0], ...). I like the way yours looks better though.
|
Thanks for looking this over. I'll add some documentation as soon as I can and will update the PR accordingly. |
|
Hey @npadmana, just checking in while trying to lower our open PR count. Do you mind either:
|
|
(I'd also be in favor of moving these tests to |
Runtime types aren't necessarily analyzed as "borrows" but domains used as runtime types need to be kept around until the end of the function. An alternative would be to mark the runtime-type variables as "aliasing" the domain. Update ex.good for change making runtime types "capturing"
see the test named err-return.chpl for example
PRIM_END_OF_STATEMENT shouldn't prevent something from being removed, so now we just allow such references to dead VarSymbols and update the between-pass cleaning to delete them before deleting the dead VarSymbols. This will prevent access to invalid memory after the VarSymbols are deleted.
They were deprecated in 13888.
We know the implementation for atomic add/sub is tricky in a lot of configurations, so add a stress test. atomic add/sub for reals uses a CAS loop for intrinsics/cstdlib, and there's several implementations in ugni (-nl 1, NIC ops, not registered, and Gemini vs. Aries differences.)
Just use the standard processor atomic implementation instead of the custom ones. The custom ones were originally added years ago before we had a standard processor atomic implementation.
Just use the standard processor atomic implementation instead of the custom ones. The custom ones were originally copied from ugni, which had them before we had a standard processor atomic implementation.
Add isCopyable etc Motivated by work in PR chapel-lang#14835, I realized that `list` needs to avoid having an `init=` or `=` function when the element type is a non-nilable owned, e.g. `owned C`. The problem is that in that event, the source list elements would be transferred out of and then store `nil`. Additionally `list` should not include an `init=` function if the elements do not include one. In order to solve that problem, `list` will need to be able to query whether or not the elements are copyable. In addition it makes sense to be able to query if a type is only copyable from a `ref`, which is the case for `owned C?` (say). This PR adds the following queries to the Types standard module: * `isConstCopyable` returns true if the type can be initialized from a `const` value (`var x = myConstValue`) * `isCopyable` returns true if the type can be copy-initialized at all (but it might only be initializable from a mutable other variable, as is the case with `owned`) * `isConstAssignable` returns true if the type can be assigned from a `const` value (`x = myConstValue`) * `isAssignable` returns true if the type can be assigned at all (but it might only be assignable from a mutable rhs variable, as is the case with `owned`). * `isDefaultInitializable` returns true if the type has a default value (e.g. supports `var z: MyType;`) `isCopyable` and `isConstCopyable` are chosen because some types (including `map` after PR chapel-lang#14793) use the ref-if-modified intent in `init=`. For these types, it is not known at resolution time whether the intent will be `ref` or `const ref`. This PR adds the new functions to Types.chpl and the related primitives and support in preFold. Flags are computed and applied to a type so that multiple calls to `isCopyable` will not result in trying to resolve `init=` each time. Additionally, this PR adjusts the expiring value analysis to avoid visiting task functions twice and tidies some whitespace in preFold.cpp. Reviewed by @vasslitvinov and @lydia-duncan - thanks! - [x] full local testing Future Work: * chapel-lang#14854 - error with isDefaultInitializable applied to records containing non-nilable owned
Remove custom processor atomic add/sub for reals in ofi/ugni [reviewed by @gbtitus] Just call the standard processor atomic implementation for add/sub on reals instead of using custom ones. The custom ones were created years ago before we had a standard processor atomic implementation, but are no longer needed. The ofi ones were just copied over from ugni.
Remove available-by-default atomic peek/poke [reviewed by @gbtitus and @mppf] They were deprecated in chapel-lang#13888.
Grammar fixes [contributed by @Aniket21mathur, reviewed by Lydia] Some language fixes that I came across when reading the partialInstantiation doc.
CHPL_LIB_PIC should be set when running on non-Mac systems
Since 14691, there are duplicate/consecutive `chpl_after_forall_fence()` calls for check-fence-after-forall. For now we're accepting them to quiet the test since it's going to run under ugni nightly testing again. See Cray/chapel-private#745 for more info.
…after-forall Accept duplicate chpl_after_forall_fence() for check-fence-after-forall [reviewed by @mppf] Since chapel-lang#14691, there are duplicate/consecutive `chpl_after_forall_fence()` calls for check-fence-after-forall. For now we're accepting them to quiet the test since it's going to run under ugni nightly testing again. See Cray/chapel-private#745 for more info.
Extend python interoperability skipif for when CHPL_LIB_PIC not set [double checked with Elliot] @ronawho was encountering test failures with cython and numpy installed on a linux system due to not having CHPL_LIB_PIC set. This avoids those failures while not preventing testing from running. Verified behavior on: - darwin - linux64 without CHPL_LIB_PIC and without cython & numpy installed - linux64 with CHPL_LIB_PIC - linux64 without CHPL_LIB_PIC - linux64 and gasnet with CHPL_LIB_PIC - linux64 and gasnet without CHPL_LIB_PIC
Add list performance test (chapel-lang#14821) This PR adds a simple list performance test that mimics the structure of `timeVectorArray.chpl`, a regression test for array-as-vector operations that was removed in chapel-lang#14557. --- Testing: - [x] listBenchmark1.chpl on linux64 when CHPL_COMM=none - [x] listBenchmark1.chpl on linux64 when CHPL_COMM=none, -perf - [x] Graph renders properly when generated locally --- Thanks to @benharsh, @ronawho for review!
This ensures that the character set environment we've set up on the login node is correctly propagated to the compute node. This will resolve failures we're seeing on an XE. We set LC_ALL and friends in start_test, but that was getting overridden by a login script that is sourced when the qsub session starts.
…g#14794) I found some small mistakes while reading the owned documentation, so fix them. This was initially a slightly bigger change but PR chapel-lang#14829 updated some of the fixes. [reviewed by @mppf]
…-charset Propagate character set env vars in chpl_launchcmd [reviewed by @mppf] This ensures that the character set environment we've set up on the login node is correctly propagated to the compute node. This will resolve failures we're seeing on an XE. We set LC_ALL and friends in start_test, but that was getting overridden by a login script that is sourced when the qsub session starts.
Our provider handling has been a bit slapdash, and with work on unordered atomics about to start this is a good time to clean it up. So here, add a provider set type and operations upon it, then rearrange and rewrite so as to do checks against available and in-use providers in the same way. Then, get rid of network-based checks. These weren't necessarily accurate anyway and are better done as provider-based checks. For example, the part of the check in init_fixedHeap() for needing a fixed heap when using the gni provider on a Cray XC system was effectively ignoring what provider would be used, and we don't need a fixed heap if we're using the tcp provider. While here, improve the getinfo-related debug support.
…ator-bug Future showing compiler segfault when capturing an iterator with no yields [trivial, not reviewed] This future demonstrates that when an iterator with no yields is captured as an array, it causes the compiler to segfault.
…-handling More-principled provider handling in comm=ofi. (Reviewed by @ronawho.) Our provider handling has been a bit slapdash, and with work on unordered atomics about to start this is a good time to clean it up. So here, add a provider set type and operations upon it, then rearrange and rewrite so as to do checks against available and in-use providers in the same way. Then, get rid of network-based checks. These weren't necessarily accurate anyway and are better done as provider-based checks. For example, the part of the check in init_fixedHeap() for needing a fixed heap when using the gni provider on a Cray XC system was effectively ignoring what provider would be used, and we don't need a fixed heap if we're using the tcp provider. While here, also improve the fi_getinfo-related debug support.
|
@ronawho, @daviditen -- I (finally!) just pushed over some documentation for this. Unfortunately, I had also rebased this branch, so the list of changes looks horrible. So I might just go ahead and create a new branch for this, with a more limited list of changes. Once I do this, I'll go ahead and close this.... |
|
Closing in favor of #14873. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This demonstrates a prototype version for tables using HDF5. Currently, this is rather minimal -- providing simple routines to create and append to tables, and to read a subset of columns of a table.
The underlying record can have basic integer and real numeric types, as well as
c_arraytypes.I've currently marked this directory NOTEST, since the current HDF5 module requires MPI (and I'm not sure the best way to set that up).
This addresses #14550.