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

fix(runtime): explicitly load previous results #106

Merged
merged 2 commits into from
Jun 11, 2024
Merged

Conversation

eddiebergman
Copy link
Contributor

Test by rerunning the reproducible examples (Many thanks @danrgll!)


The issue was from my previous re-working runtime, where I forgot to ensure that if there was a trial (i.e. config_1_1) that relied on a previous trial (config_1_0), to ensure that it was loaded into that cache before config_1_1 was loaded in. Main fix was just to sort accordingly and then process each one 1-by-1.

        # NOTE: We sort all trials such that we process previous trials first, i.e.
        # if trial_3 has trial_2 as previous, we process trial_2 first, which
        # requires trial_1 to have been processed first.
        def _depth(trial: Trial.Disk) -> int:
            depth = 0
            previous = trial.previous_config_id
            while previous is not None:
                depth += 1
                previous_trial = _disk_lookup.get(previous)
                if previous_trial is None:
                    raise RuntimeError(
                        "Previous trial not found on disk when processing a trial."
                        " This should not happen as if a tria has a previous trial,"
                        " then it should be present and evaluated on disk.",
                    )
                previous = previous_trial.previous_config_id

            return depth

        # This allows is to traverse linearly and used cached values of previous
        # trial data loading, as done below.
        _disks.sort(key=_depth)

        for disk in _disks:
            # process the trial

PR also attempts to simplify things a bit and reduce some duplication, as fixing this issue would have introduced a lot of spaghetti.

@eddiebergman eddiebergman merged commit a2d0fb1 into master Jun 11, 2024
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant