[Usage stats] Record Ray native library usage from a home temp folder #25842
Conversation
| # We need a file lock in order to | ||
| # Set the short timeout to avoid too long delay in import. | ||
| with TempFileLock(str(self.lib_usage_file), timeout=2): | ||
| with self.lib_usage_file.open("a+") as f: |
There was a problem hiding this comment.
Don't use a file lock, just touch separate files for each key.
There was a problem hiding this comment.
I think this still can have some corruption (race condition) if there's more than one instance or driver running concurrently.
There was a problem hiding this comment.
Touch a separate file (don't write anything, just create it), for each library. They can have separate file names.
There was a problem hiding this comment.
Basically, mirror the kv put strategy.
There was a problem hiding this comment.
Ah, I see. That's a great idea. Let me follow up
There was a problem hiding this comment.
Addressed. It seems like Windows doesn't support atomic file creation, so I used Filelock there. For Linux, it seems like PathLib().touch() uses exclusive open, so it seems to be safe to just use touch(exist_ok=True)
ericl
added
@author-action-required
| if lib_name not in libs: | ||
| f.seek(0, io.SEEK_END) | ||
| self._write(f, lib_name) | ||
|
|
There was a problem hiding this comment.
TODO add docstring
| ray._private.utils.get_ray_temp_dir() | ||
| ).read() | ||
| for library_usage in historical_lib_usages: | ||
| if library_usage not in result: |
There was a problem hiding this comment.
The overhead should be pretty small as there are only 5 libs
python/ray/util/ml_utils/filelock.py
Outdated
| import tempfile | ||
| from pathlib import Path | ||
|
|
||
| if sys.platform == "win32": |
There was a problem hiding this comment.
I realized the basic FileLock is Unix only
| # We need a file lock in order to | ||
| # Set the short timeout to avoid too long delay in import. | ||
| with TempFileLock(str(self.lib_usage_file), timeout=2): | ||
| with self.lib_usage_file.open("a+") as f: |
There was a problem hiding this comment.
I think this still can have some corruption (race condition) if there's more than one instance or driver running concurrently.
|
It will be ready by today! |
rkooo567
removed
the
@author-action-required
ericl
added
the
@author-action-required
|
Working on fixing the weird test failure + Windows issue |
rkooo567
removed
the
@author-action-required
ray-project#25842 is not needed since we will no longer accidentally create a new cluster while an existing one is running after ray-project#26678 Signed-off-by: Edward Oakes <[email protected]>
ray-project#25842 is not needed since we will no longer accidentally create a new cluster while an existing one is running after ray-project#26678
ray-project#25842 is not needed since we will no longer accidentally create a new cluster while an existing one is running after ray-project#26678 Signed-off-by: elliottower <[email protected]>
Why are these changes needed?
This PR records the historical Ray native library usage to the home temp folder. Note that library usage only includes Ray native libraries (rllib, tune, dataset, workflow, and train). NOTE: The library usage is always recorded to /tmp/ray, but they will only recorded when the cluster that enables the usage stats is running. Note that this can generate quite big amount of false positive (e.g., If I import rllib once, and start cluster for local development, they will all considered as a rllib cluster).
Related issue number
Checks
scripts/format.shto lint the changes in this PR.