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

Lingo: Optimize imports and remove unused parameter #4305

Merged
merged 7 commits into from
Dec 3, 2024

Conversation

nicholassaylor
Copy link
Contributor

What is this fixing or adding?

See above

How was this tested?

Running unittests and generation

Ignore the messy git history

@nicholassaylor
Copy link
Contributor Author

Requesting review from @hatkirby

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Nov 30, 2024
@hatkirby
Copy link
Collaborator

Did you test pickling? Running the command in the readme (python worlds\lingo\utils\pickle_static_data.py) gives the following error:

Traceback (most recent call last):
  File "C:\Users\feffe\Documents\Repos\Archipelago\worlds\lingo\utils\pickle_static_data.py", line 4, in <module>
    from ..datatypes import Door, DoorType, EntranceType, Painting, Panel, PanelDoor, Progression, Room, RoomAndDoor, \
ImportError: attempted relative import with no known parent package

@Exempt-Medic Exempt-Medic added waiting-on: author Issue/PR is waiting for feedback or changes from its author. is: refactor/cleanup Improvements to code/output readability or organizization. labels Nov 30, 2024
@hatkirby hatkirby removed the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Dec 1, 2024
@nicholassaylor
Copy link
Contributor Author

Did you test pickling? Running the command in the readme (python worlds\lingo\utils\pickle_static_data.py) gives the following error:

Traceback (most recent call last):
  File "C:\Users\feffe\Documents\Repos\Archipelago\worlds\lingo\utils\pickle_static_data.py", line 4, in <module>
    from ..datatypes import Door, DoorType, EntranceType, Painting, Panel, PanelDoor, Progression, Room, RoomAndDoor, \
ImportError: attempted relative import with no known parent package

Using python -m worlds.lingo.utils.pickle_static_data from the base Archipelago directory fixes that issue. Would you rather me change the README to reflect that or revert the changes? If I revert, the PR effectly becomes one just for the removed parameter, which is fine if that is what you'd prefer. I fully intend to defer to your judgement on this.

@hatkirby
Copy link
Collaborator

hatkirby commented Dec 1, 2024

The problem with python -m is that it loads every package on the way to the target one, which means that it has to initialize the Lingo world. If generated.dat already exists and you're updating it, this works fine, but if it doesn't, you get this error:

ERROR:root:Could not load world WorldSource(lingo, is_zip=False, relative=True):
Traceback (most recent call last):
  File "C:\Users\feffe\Documents\Repos\Archipelago\worlds\__init__.py", line 83, in load
    importlib.import_module(f".{self.path}", "worlds")
  File "C:\Users\feffe\AppData\Local\Programs\Python\Python312\Lib\importlib\__init__.py", line 90, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1331, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 935, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 995, in exec_module
  File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
  File "C:\Users\feffe\Documents\Repos\Archipelago\worlds\lingo\__init__.py", line 10, in <module>
    from .items import ALL_ITEM_TABLE, ITEMS_BY_GROUP, TRAP_ITEMS, LingoItem
  File "C:\Users\feffe\Documents\Repos\Archipelago\worlds\lingo\items.py", line 5, in <module>
    from .static_logic import DOORS_BY_ROOM, PROGRESSIVE_ITEMS, get_door_group_item_id, get_door_item_id, \
  File "C:\Users\feffe\Documents\Repos\Archipelago\worlds\lingo\static_logic.py", line 140, in <module>
    load_static_data_from_file()
  File "C:\Users\feffe\Documents\Repos\Archipelago\worlds\lingo\static_logic.py", line 110, in load_static_data_from_file
    file = pkgutil.get_data(__name__, os.path.join("data", "generated.dat"))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\feffe\AppData\Local\Programs\Python\Python312\Lib\pkgutil.py", line 453, in get_data
    return loader.get_data(resource_name)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap_external>", line 1186, in get_data
FileNotFoundError: [Errno 2] No such file or directory: 'C:\\Users\\feffe\\Documents\\Repos\\Archipelago\\worlds\\lingo\\data\\generated.dat'

The static_data module could be updated to check whether the file exists and not initialize the data if it doesn't, but that causes further problems because the items and locations modules try to look up IDs when initializing, and that requires more cascading fixes. It is unlikely that one would end up in a situation where there's no valid generated.dat pickle available in the directory, but IMO it isn't good to have a bootstrap problem like this.

I also think it is just a lot of unnecessary overhead to have to load and initialize the world in order to run this utility, even if the datafile does already exist. This is an edge case that is unfortunately a bit tricky to work around in Python and it's how we ended up with this clunky solution. Unless you know of a way to restructure things to avoid all of these issues, I think it would be best to revert that part and only remove the extra parameter in player_logic (thanks for noticing that btw!).

@nicholassaylor
Copy link
Contributor Author

Thank you for taking the time to explain everything, it was very helpful. I have reverted the import change, but while doing so, I found a duplicate import sys which I removed. There may be some way to move the imports to __init__.py to solve the "errors" that Pycharm raises when inspecting the code, but I think that since this just works anyway, it's not work the effort.

@Exempt-Medic Exempt-Medic added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: author Issue/PR is waiting for feedback or changes from its author. labels Dec 2, 2024
@nicholassaylor nicholassaylor changed the title Lingo: Simplify datatype imports and remove unused parameter Lingo: Optimize imports and remove unused parameter Dec 2, 2024
@Berserker66 Berserker66 merged commit 6f2e1c2 into ArchipelagoMW:main Dec 3, 2024
16 checks passed
@nicholassaylor nicholassaylor deleted the lingo-imports branch December 3, 2024 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: refactor/cleanup Improvements to code/output readability or organizization. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants