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

Agent IDs #512

Merged
merged 2 commits into from
May 13, 2021
Merged

Agent IDs #512

merged 2 commits into from
May 13, 2021

Conversation

Robadob
Copy link
Member

@Robadob Robadob commented Apr 13, 2021

Plan of Action

  • Id type is a typedef
  • Agent's all contain variable _id
  • Expose global thread index on device API (but how to name it?)
  • This cannot be modified directly via
    • AgentDescription
    • AgentVector::Agent
    • HostNewAgentAPI
    • DeviceAPI (with seatbelts)
    • HostAgentAPI (Not applicable, none of these methods change agent data, beyond sorting agents)
  • Agent ID collision is detected and an exception is thrown.
    • When CUDASimulation::setPopulationData() is called
    • When agents are loaded from file (This triggers the same CUDAAgent::setPopulationData() which performs the check)
  • Collision detection uses a device API technique for large agent populations? (agent data is already on the device, so the method is always pure-device, only concern is that 3 cudamalloc/cudafree are used per call might be able to share these with stream resources somehow. Also it currently uses default stream...)
  • Either
    • AgentVector has a way to clear IDs
    • CUDASimulation::setAgentPopulation()/file loader has an optional argument to reset IDs.
  • Agent ID can be accessed via dedicated getID() method
    • AgentVector::Agent
    • HostNewAgentAPI
    • DeviceAPI
    • DeviceNewAgentAPI
  • Calling CUDASimulation::simulate(), agent's with an unset ID are assigned an ID prior to init functions executing
  • Calling CUDASimulation::step() assigns agent ids on first run, if not already assigned.
  • HostNewAgentAPI agents have an ID generated instantly
  • DeviceAgentVector new agents have an ID generated instantly
  • AgentVector insert/copy methods all reset copied id to unset flag
  • Agents birthed on device receive an ID instantly (based on atomicInc)
  • Do submodels resetting handle agentIDs correctly? (e.g. if submodel only agent pop is reset, do they have ID counter reset).
  • Do submodels auto bind _id for any bound agents?
  • Add stream support and device buffer reuse to CUDAAgent::validateIDCollisions() (Concluded not really worthwhile)
  • Add stream support and device buffer reuse to CUDAFatAgent::assignIDs()
  • Add a flag to reset AgentIDs when loading from file??
  • Review HostNewAgentAPI copy/assign behaviour. Block agent type mixing, and gen new ID (how?). (Existing copy/assign copy a pointer not the data, so copies create copy interfaces to the same data).
  • Update examples to use internal ID
  • C tests
  • Py tests
  • Documentation (mostly user guide, rather than api docs) (I've put relevant info into an issue)
  • Move tests into most appropriate suites (from the single suite used for development)

Closes #199 (when finished)
Replaces #240 (now closed)

@Robadob Robadob marked this pull request as draft April 13, 2021 13:56
@Robadob Robadob self-assigned this Apr 13, 2021
@ptheywood ptheywood mentioned this pull request Apr 13, 2021
35 tasks
@Robadob
Copy link
Member Author

Robadob commented Apr 13, 2021

Copying these ideas from #240

  • Typedef for the type to simplify changing this (AgentData::IDtype)

  • Remove explicit id from examples, to make use of the new feature.

  • ~ Documentation (although in general we need to smash userguide)


Questions from #240

Should this be written and read from disk? This increases complexity.
Should _ props be read/written in general? (no?)

Yes, some models will have agents track other agent ids (e.g. @MILeach's scharr disease model)

When creating host populations, how should id be handled. Should there be a population relative ID which are then mutated when the pop is added to a cuda agent model? This will need thinking about.

This is already answered in #199, IDs are initialised the first time an agent part of an CUDASimulation which executes init functions or steps. Imported agent pops are a special case, where an exception is thrown in case of conflict, with some workaround to purge IDs.

do we want this to be optionally disabled to save memory? (probably not)

No, this just adds complication, to save a small amount of mem, worst case we can fiddle with the typedef to reduce var size.

Per agent-type unique Ids, or globally unique Ids?

Per agent.

index vs ID.

ID because there is no guarantee that they form a contiguous block (or begin from 1) which is what I feel is implied by index. The only guarantee is that they are unique (assuming they don't exceed UINT_MAX).

@ptheywood
Copy link
Member

ID because there is no guarantee that they form a contiguous block (or begin from 1) which is what I feel is implied by index. The only guarantee is that they are unique (assuming they don't exceed UINT_MAX).

I think this comment/question was more that we need to make it clear its an ID, and that we might also want to expose the index within a population to the user.

@Robadob
Copy link
Member Author

Robadob commented Apr 13, 2021

I think this comment/question was more that we need to make it clear its an ID, and that we might also want to expose the index within a population to the user.

Ah that makes sense.

We already have an internal global thread index method on DeviceAPI. Exposing that to the user is a trivial change. Just need to be clear about how we name it, given we're trying to abstract that level of detail from the average user.

I can make that part of this PR too.

@Robadob Robadob force-pushed the agent_id branch 3 times, most recently from 2bea090 to 2855bc4 Compare April 14, 2021 15:32
@Robadob
Copy link
Member Author

Robadob commented Apr 28, 2021

Have discussed how device agent birth id's should work.

Agreed to use atomics (contrary to an earlier decision), and each CUDAFatAgent (which manages the handing out of IDs), will host a device memory mirror of it's nextID variable. This is what will be atomically incremented. By tracking it within CUDAFatAgent, it should be possible to avoid needing to update d_nextID, when nextID has not changed since it was last updated (e.g. no host agent birth).

@Robadob
Copy link
Member Author

Robadob commented Apr 29, 2021

With the latest changes (and this Jitify fix for #include <limits>), the SWIG boids example now runs.

As noted in it's commit message, 7ae4a88 will likely be reverted in future and swapped for a version where the typedef is moved.

Lint is even failing, because it's unhappy that I moved the includes of AgentData.h inside #ifndef __CUDACC_RTC__

@Robadob
Copy link
Member Author

Robadob commented Apr 30, 2021

Rough test plan

  • Agent's IDs are unique (create a handful of agents via every method, export all agents and check when added to a std::set they are unique).
  • Agent ID collision is detected on import from file
  • Agent IDs are exported/imported
  • Agent ID can be read (via every method) (Reading via device API is covered in tests that ID is actually set at device birth).
  • DeviceAgentBirth ID matches actual given ID
  • AgentVector ID interaction?
  • Submodel binds ID
  • Submodel unbound agents id counter resets
  • Device error on agent_out.getID() if agent out not enabled.

@Robadob
Copy link
Member Author

Robadob commented May 11, 2021

I've added Message<Type>Description::newVariableID() to swig as a wrapper for Message<type>Description::newVariable<id_t>() which itself is currently an alias for unsigned int. Passing IDs to messages is used quite frequently in examples. Less clear if need to add a similar alias to all the SWIG environment methods.

@Robadob
Copy link
Member Author

Robadob commented May 12, 2021

Final Changes

  • Swigify ID type for agent var/envproperty
  • Add some tests for py ID type
  • Split agent_id tests into most relevant suites
  • Squash

Copy link
Member

@ptheywood ptheywood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skimmed the diff (mostly to see why it was so large, forgot there were some tracked XML files) and generally looks fine other than the .gv files which are being tracked (that I don't believe should be).

tests/twodeps.gv Outdated Show resolved Hide resolved
tests/singlechain.gv Outdated Show resolved Hide resolved
tests/host_functions.gv Outdated Show resolved Hide resolved
tests/diamond.gv Outdated Show resolved Hide resolved
tests/all_dependencies.gv Outdated Show resolved Hide resolved
This is managed by FLAMEGPU and can be accessed with getID() in all places where getVariable is available.
IDs are assigned when a simulation begins, but can be exported and reimported to/from AgentVector/file.
Users cannot change or disable agent IDs.
id_t is the type of ID, is currently a typedef to unsigned int, but can be changed to uint64_t if desired.
If an id_t type agent/message/env variable is required in Python, newVariableID() or similar can be used.

Closes #199
@ptheywood ptheywood self-requested a review May 13, 2021 17:46
@Robadob Robadob marked this pull request as ready for review May 13, 2021 17:52
Follow up to 0ccde81, C tests were leaving artifacts on Windows.

Added dep graph outputs to gitignore, so they shouldn't get caught by mistake in future.
Copy link
Member

@ptheywood ptheywood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests all pass under linux (Release, Seatbelts=ON). 5 disabled tests are expected (performance tests which would currently fail, but should be re-enabled in the future).

[==========] 811 tests from 57 test suites ran. (481914 ms total)
[  PASSED  ] 811 tests.

  YOU HAVE 5 DISABLED TESTS
469 passed in 1015.62s (0:16:55)

@Robadob Robadob merged commit e11a2ce into master May 13, 2021
@ptheywood ptheywood deleted the agent_id branch October 29, 2021 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Agent ID generation (host and device)
2 participants