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

Workspace.from_url imports all .py files when creating LocalWorkspace #606

Open
jmerullo opened this issue Oct 15, 2023 · 1 comment
Open
Labels
bug Something isn't working

Comments

@jmerullo
Copy link

🐛 Describe the bug

When using Workspace.from_url() to create a local workspace, all other python files in the directory will be imported in the process. As a result, the code (outside of an if __main__ block) in those scripts will be run.

Here's an example:

Directory structure:
./tango_workspace/
./exp.py
./other_file.py

exp.py:

from tango import step
from tango.workspace import Workspace

@step(cacheable=True, deterministic=True)
def test_step():
    return 19


if __name__=='__main__':
    ws = Workspace.from_url('local://tango_workspace')
    print('ws', ws)
    print('result', test_step().result())

other_file.py:

#some unrelated script that does stuff
print("hello world")

running python exp.py will try to create the workspace, but in the process will import other_file.py, causing it to run and output "hello world". This is obviously a big problem if there are other python scripts in the directory that shouldn't be run.

There are a few workarounds. I believe the issue stems from this line in the cls.by_name call. I haven't tried to fully understand this code, but I think since LocalWorkspace is not in the registry, it causes this behavior. Therefore some workarounds that solve this issue are to import LocalWorkspace somewhere before creating the workspace:

In exp.py:

from tango.workspace import Workspace
from tango.workspaces.local_workspace import LocalWorkspace
...

But a more permanent solution is to add:
from .workspaces import LocalWorkspace
to the bottom of tango/__init__.py but I don't know if this is the best way to address this problem.

I am using version 1.2.1 on python 3.8.17. I think this issue would persist on 1.3 though

Versions

Python 3.8.17
ai2-tango==1.2.1

@jmerullo jmerullo added the bug Something isn't working label Oct 15, 2023
@apoorvkh
Copy link
Contributor

Big +1! I also noticed significant unexpected behavior (i.e. unwanted code being executed) when using Tango. This explains it. Thanks @jmerullo!

Imo, all built-in implementations (LocalWorkspace, etc) should be registered by default. And I really don't think Tango should be importing my local files/modules in search of un-registered classes, as this (i.e. running all project code outside of if __name__ == '__main__' blocks) is unexpected and could be dangerous. I think the user should be responsible for importing the (non-default) Registrables they need.

https://github.com/allenai/tango/blob/26a162ab133b07bf34aff21d48a80c484e5b04e3/tango/common/registrable.py#L216C8-L239

Please consider, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants