Skip to content

Remove uses of OrderedDict - #2046

Closed
sid-kap wants to merge 2 commits into
pyro-ppl:masterfrom
sid-kap:no-ordered-dict-2
Closed

Remove uses of OrderedDict#2046
sid-kap wants to merge 2 commits into
pyro-ppl:masterfrom
sid-kap:no-ordered-dict-2

Conversation

@sid-kap

@sid-kap sid-kap commented Jul 16, 2025

Copy link
Copy Markdown

Since python 3.6 dict has the same behavior as OrderedDict. Numpyro requires python version >=3.9, so it should be fine to use dict instead of OrderedDict.

@fehiepsi

Copy link
Copy Markdown
Member

Thanks @sid-kap! Do you think jax-ml/jax#24398 is still relevant for the changes?

@sid-kap

sid-kap commented Jul 16, 2025

Copy link
Copy Markdown
Author

I think it should be fine because dict now keeps the keys ordered, like OrderedDict

@fehiepsi

Copy link
Copy Markdown
Member

yes, dict in python keeps key order, but not under jit compiling

import jax

x = {"b":1,"a":2}
jax.jit(lambda x: x)(x)

returns a new order.

@sid-kap

sid-kap commented Jul 17, 2025

Copy link
Copy Markdown
Author

Ah you're right, my bad! Thanks for the clarification! It looks like jax handles OrderedDict correctly:

import jax
x = OrderedDict([("b", 1), ("a", 2)])
jax.jit(lambda x: x)(x)

returns

OrderedDict([('b', Array(1, dtype=int32, weak_type=True)), ('a', Array(2, dtype=int32, weak_type=True))])

So any function that may be jitted should be kept as OrderedDict.

I'll read through the code again and see if there are any functions that are guaranteed to not be jitted, but if not it probably makes sense to close this.

@fehiepsi

Copy link
Copy Markdown
Member

Looking at the changes, I think most are valid. We just need to keep TraceT as-is because order in trace is important for inference algorithms.

@fehiepsi fehiepsi added the awaiting review Awaiting review from maintainers label Jul 17, 2025
Comment thread numpyro/_typing.py

Message: TypeAlias = dict[str, Any]
TraceT: TypeAlias = OrderedDict[str, Message]
TraceT: TypeAlias = dict[str, Message]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's keep TraceT as OrderedDict

def _find_slps(
self, rng_key: jax.Array, *args: Any, **kwargs: Any
) -> dict[str, OrderedDictType]:
) -> dict[str, dict]:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure but it seems that we need to use OrderedDict in this module.

self,
rng_key: jax.Array,
branching_trace: OrderedDictType,
branching_trace: dict,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

like above, maybe we need to use OrderedDict in this module.

Comment thread numpyro/handlers.py

>>> exec_trace = trace(seed(model, random.PRNGKey(0))).get_trace()
>>> pp.pprint(exec_trace) # doctest: +SKIP
OrderedDict([('a',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

revert this

Comment thread numpyro/util.py
# Restrict cache size to prevent ref cycles.
max_size = 8
outer_fn._cache = getattr(outer_fn, "_cache", OrderedDict())
outer_fn._cache = getattr(outer_fn, "_cache", {})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's use OrderedDict here I guess.

@fehiepsi fehiepsi added WIP Work In Progress and removed awaiting review Awaiting review from maintainers labels Aug 4, 2025
@sid-kap

sid-kap commented Aug 22, 2025

Copy link
Copy Markdown
Author

Ok sorry, it sounds like most of this will need to be undone. I will close for now

@sid-kap sid-kap closed this Aug 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

WIP Work In Progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants