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

Tests: add a test that checks the state of the multiworld for determinism #2819

Closed

Conversation

alwaysintreble
Copy link
Collaborator

What is this fixing or adding?

Title. also adds a seed arg to setup_solo_multiworld

How was this tested?

tests

If this makes graphical changes, please attach screenshots.

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Feb 14, 2024
@alwaysintreble alwaysintreble added is: enhancement Issues requesting new features or pull requests implementing new features. affects: core Issues/PRs that touch core and may need additional validation. labels Feb 14, 2024
@alwaysintreble
Copy link
Collaborator Author

@lordlou @Alchav pinging you since i can't request reviews from you

@ScipioWright ScipioWright added the waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. label Feb 14, 2024
@beauxq
Copy link
Collaborator

beauxq commented Feb 14, 2024

I think the issue with Zillion is in core Options.py line 399 - using global random.
The multiworld.set_seed in the test doesn't seed global random.

(This could be a problem with other games if they have default random options.)

This is without looking really closely. Maybe I'll have more time to look more closely later.

@beauxq
Copy link
Collaborator

beauxq commented Feb 14, 2024

Note that Generate.py seeds global on line 80

random.seed(seed)
so it is fragilely deterministic.

To more accurately test this, you could, in setup_solo_multiworld:

     multiworld.game[1] = world_type.game
     multiworld.player_name = {1: "Tester"}
+    random.seed(seed)
     multiworld.set_seed(seed)
     multiworld.state = CollectionState(multiworld)

@beauxq
Copy link
Collaborator

beauxq commented Feb 14, 2024

It looks like it's the same issue with Mystic Quest, because the differing item in the test results always involves a weapon that can be a starting weapon. And Mystic Quest StartingWeapon option is default = "random"

@alwaysintreble
Copy link
Collaborator Author

to make it most closely mimic normal generation it should probably be

    multiworld.player_name = {1: "Tester"}
    random.seed(seed)
    multiworld.state = CollectionState(multiworld)
    args = Namespace()
    for name, option in world_type.options_dataclass.type_hints.items():
        setattr(args, name, {1: option.from_any(option.default)})
    multiworld.set_seed(seed)

should it reinitialize the global random after to prevent worlds from using that? currently global random and multiworld random are the same object but there's not really a guarantee that stays the same and using the global one is even more unsafe than the multiworld random

@github-actions github-actions bot removed the affects: core Issues/PRs that touch core and may need additional validation. label Feb 14, 2024
@Exempt-Medic Exempt-Medic added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Mar 12, 2024
@alwaysintreble alwaysintreble removed the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Mar 12, 2024
@alwaysintreble
Copy link
Collaborator Author

this is failing unit tests because of super metroid.

@Berserker66
Copy link
Member

preferably global random doesn't need to be seeded for AP generation, so adding global seeding to the test goes in the wrong direction.

@beauxq
Copy link
Collaborator

beauxq commented Mar 12, 2024

It seems like it would take a lot of work to remove global random from Options.py

I think it would be good to do, but I don't think this unit test should wait on that.
But Super Metroid could take a while, so maybe it's a race between fixing core and fixing Super Metroid?

We could have a comment on the seeding line in the unit test TODO: remove after global random is removed from Options.py

# Conflicts:
#	test/general/__init__.py
@Exempt-Medic Exempt-Medic added the waiting-on: author Issue/PR is waiting for feedback or changes from its author. label Jul 28, 2024
@Exempt-Medic Exempt-Medic added waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. waiting-on: other Issue/PR is waiting for something else, like another PR. and removed waiting-on: author Issue/PR is waiting for feedback or changes from its author. waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. labels Nov 25, 2024
@Exempt-Medic
Copy link
Member

Seems like it's failing for some other reason now

@Exempt-Medic Exempt-Medic added waiting-on: author Issue/PR is waiting for feedback or changes from its author. and removed waiting-on: world-maintainer Issue/PR is waiting for feedback or approval by the maintainer of a world. labels Nov 28, 2024
@Exempt-Medic
Copy link
Member

Leaving a comment to explain the tag: This has to account for Zillion and Super Metroid or whatever it was.

@beauxq
Copy link
Collaborator

beauxq commented Nov 28, 2024

Although Zillion does use global random, and I have been working to remove that, it has not been a high priority because it does not cause any determinism problem currently.

There is currently a problem with this PR that makes every game fail. Once that is fixed, I think Zillion will be passing.

@Exempt-Medic
Copy link
Member

Exempt-Medic commented Nov 28, 2024

Although Zillion does use global random, and I have been working to remove that, it has not been a high priority because it does not cause any determinism problem currently.

There is currently a problem with this PR that makes every game fail. Once that is fixed, I think Zillion will be passing.

Right, the PR has to account for Zillion. For example, by excluding it from the test, or actually fixing the problem which was the truncation I believe

@beauxq
Copy link
Collaborator

beauxq commented Nov 28, 2024

Right, the PR has to account for Zillion. For example, by excluding it from the test, or actually fixing the problem which was the truncation I believe

I don't think there's anything specific about Zillion. That problem is making every game fail.

@Exempt-Medic
Copy link
Member

Exempt-Medic commented Nov 28, 2024

Right, the PR has to account for Zillion. For example, by excluding it from the test, or actually fixing the problem which was the truncation I believe

I don't think there's anything specific about Zillion. That problem is making every game fail.

Running the test locally, it passes for every game (except Super Metroid) if I just skip Zillion. I believe the problem is zero items or zero locations in the gen. Which isn't something Zillion is doing wrong per se, the PR just has to fix it in some way

@beauxq
Copy link
Collaborator

beauxq commented Nov 28, 2024

Running the test locally, it passes for every game (except Super Metroid) if I just skip Zillion. I believe the problem is zero items or zero locations in the gen. Which isn't something Zillion is doing wrong per se, the PR just has to fix it in some way

I just tried removing the zillion directory, and it still fails with game_name='Archipelago' It still fails with every game.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: author Issue/PR is waiting for feedback or changes from its author. waiting-on: other Issue/PR is waiting for something else, like another PR. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants