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

Missing dependency in setup.py #118

Open
jmccartin opened this issue Dec 27, 2022 · 2 comments
Open

Missing dependency in setup.py #118

jmccartin opened this issue Dec 27, 2022 · 2 comments

Comments

@jmccartin
Copy link

This is a pretty simple issue at first glance.

You have a missing dependency in setup.py for the library atomicwrites. The actual library calls are in downloader.py, for example:

with atomic_write(filepath_attempt, mode='w', overwrite=True) as wf:

This means that any project that uses laika as a dependency itself will fail unless it explicitly knows to install atomicwrites itself. But on the project homepage, the author of that package recommends its deprecation:

I thought it'd be a good time to deprecate this package. Python 3 has os.replace and os.rename which probably do well enough of a job for most usecases.

So, what's the need for that atomic_write function? Could it not be simply replaced by a basic wrapper using native library calls?

@gast04
Copy link
Contributor

gast04 commented Jan 2, 2023

thank for the comment, it was added here 6e87f53 to fix a race condition, as astro_dog downloads in parallel,
just to replace it will not work, we could add locking here, would merge a PR for this

@gregjhogan
Copy link
Member

gregjhogan commented Jan 3, 2023

I don't think atomic_write() is any better than a simple temp file with random name + rename in this situation.

For example we used to do this here which was safe:

with tempfile.NamedTemporaryFile(delete=False, dir=tmp_dir) as fout:
  fout.write(str.encode(str(unix_time)))
os.replace(fout.name, filepath + '.attempt_time')

It looks like the only race condition was caused by using hatanaka.decompress_on_disk() which is no longer used. I think a temp file with random name + rename approach should work everywhere we use atomic_write() now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants