search

commaai / laika

Simple Python GNSS processing library
MIT License
634 stars 178 forks source link

Missing dependency in setup.py #118

Open jmccartin opened 1 year ago

jmccartin commented 1 year ago

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: https://github.com/commaai/laika/blob/5eb0c3c2596dd12a232b83bdb057a716810e89cf/laika/downloader.py#L283

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 commented 1 year ago

thank for the comment, it was added here https://github.com/commaai/laika/commit/6e87f536dbe8cf80040f724c89798e66ca17cf9d 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 commented 1 year ago

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.