Repository: allenai/dolma (language: python)

Issue:
Fix Actions to skip S3 tests from external contributors
This would make the failures for #23 and #24 go away. 

Having external collaborators hit the test bucket is a security risk, so those tests should be skipped.


Discussion / hints:
This would be helpful!
Some reproducible and accessible data, even if it requires some work on the external collaborators part, could be a reasonable alternative as well.

Also, If S3 access is crucial even for outside cotnributors there are some world-readable-free aws buckets such as [PMC Open Access (OA) Subset on AWS](https://www.ncbi.nlm.nih.gov/pmc/tools/pmcaws/)  `s3://pmc-oa-opendata`
I am not sure if it is acceptable access to use it for this purpose. (If I had to guess it would be no)
That's an interesting idea! Some of the tests require buckets to be
writable, so we need to skip some anyway, but it would be nice to keep at
least some of the tests.

On Aug 21, 2023 at 06:06:40, Chris Ha ***@***.***> wrote:

> This would be helpful!
> Some reproducible and accessible data, even if it requires some work on
> the external collaborators part, could be a reasonable alternative as well.
>
> Also, If S3 access is crucial even for outside cotnributors there are some
> world-readable-free aws buckets such as PMC Open Access (OA) Subset on AWS
> <https://www.ncbi.nlm.nih.gov/pmc/tools/pmcaws/> s3://pmc-oa-opendata
> I am not sure if it is acceptable access to use it for this purpose. (If I
> had to guess it would be no)
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/allenai/dolma/issues/27#issuecomment-1686294674>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AA53OIQXNQHPKEQJTG6HBJDXWNMOBANCNFSM6AAAAAA3XSWFBM>
> .
> You are receiving this because you were assigned.Message ID:
> ***@***.***>
>

Also is this issue that is blocking the assessment and merging of those two PRs at this time?
I am planning further commits upon those changes, if they cannot be integrated before this is addressed, I would rather assist in developing or testing this issue to help move things along.

Fix patch:
diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml
index b9a06f02..fa913266 100644
--- a/.github/workflows/CI.yml
+++ b/.github/workflows/CI.yml
@@ -11,14 +11,23 @@ on:
       branches:
         - main
         - master
+      paths:
+        - '**.py'
+        - '**.rs'
+        - 'pyproject.toml'
+        - 'Cargo.toml'
    workflow_dispatch:
 
 
 permissions:
   contents: read
 
-jobs:
+env:
+  DOLMA_TEST_SKIP_AWS: ${{ secrets.AWS_ACCESS_KEY_ID == '' && 'true' || 'false' }}
+  DOLMA_TEST_S3_PREFIX_ENV_VAR: s3://dolma-tests
+
 
+jobs:
   info:
     runs-on: ubuntu-latest
     steps:
@@ -29,10 +38,11 @@ jobs:
           echo "run tests:  ${{ github.event_name == 'pull_request' || github.event_name == 'push' }}"
           echo "is main:    ${{ github.ref == 'refs/heads/main' || github.ref == 'refs/heads/master' }}"
           echo "is release: ${{ startsWith(github.ref, 'refs/tags/') }}"
+          echo "skip AWS:   ${{ env.DOLMA_TEST_SKIP_AWS }}"
           echo "commit:     ${{ github.sha }}"
-          echo "-------------------------------------------------------------------------"
-          echo "Full PR details:"
-          echo "${{ toJson(github.event.pull_request) }}"
+          echo "PR base repo:  ${{ github.event.pull_request.base.repo.full_name }}/tree/${{ github.event.pull_request.base.ref }}"
+          echo "PR head repo:  ${{ github.event.pull_request.head.repo.full_name }}/tree/${{ github.event.pull_request.head.ref }}"
+
 
   tests:
     runs-on: ubuntu-latest


Test patch:
diff --git a/tests/python/test_deduper.py b/tests/python/test_deduper.py
index 19d31e13..243198cc 100644
--- a/tests/python/test_deduper.py
+++ b/tests/python/test_deduper.py
@@ -12,6 +12,7 @@
     download_s3_prefix,
     get_test_prefix,
     load_jsonl,
+    skip_aws_tests,
     upload_s3_prefix,
 )
 
@@ -22,11 +23,17 @@
 class TestDeduper(TestCase):
     def setUp(self) -> None:
         self.stack = ExitStack()
-        self.test_prefix = get_test_prefix()
         self.local_temp_dir = self.stack.enter_context(TemporaryDirectory()).rstrip("/")
 
-        # upload test data
-        upload_s3_prefix(s3_prefix=f"{self.test_prefix}", local_prefix="tests/data/provided/documents/*.gz")
+        if skip_aws_tests():
+            self.remote_test_prefix = None
+        else:
+            self.remote_test_prefix = get_test_prefix()
+
+            # upload test data
+            upload_s3_prefix(
+                s3_prefix=f"{self.remote_test_prefix}", local_prefix="tests/data/provided/documents/*.gz"
+            )
 
         # copy provided config files to local temp dir
         shutil.copytree(
@@ -36,7 +43,8 @@ def setUp(self) -> None:
         )
 
     def tearDown(self) -> None:
-        clean_test_data(self.test_prefix)
+        if self.remote_test_prefix is not None:
+            clean_test_data(self.remote_test_prefix)
         self.stack.close()
 
     def test_dedupe_by_url(self):
@@ -76,10 +84,13 @@ def test_dedupe_paragraphs(self):
         self.assertEqual(expected, computed)
 
     def test_dedupe_by_url_remote_input(self):
+        if self.remote_test_prefix is None:
+            return self.skipTest("Skipping AWS tests")
+
         with open(DEDUPE_BY_URL, "r") as f:
             config = json.load(f)
 
-        config["documents"][0] = f'{self.test_prefix}/{config["documents"][0]}'
+        config["documents"][0] = f'{self.remote_test_prefix}/{config["documents"][0]}'
         config["bloom_filter"]["file"] = f'{self.local_temp_dir}/{config["bloom_filter"]["file"]}'
 
         with NamedTemporaryFile("w") as f:
@@ -88,7 +99,7 @@ def test_dedupe_by_url_remote_input(self):
 
             main(argv=["-c", f.name, "dedupe"])
 
-        download_s3_prefix(self.test_prefix, self.local_temp_dir)
+        download_s3_prefix(self.remote_test_prefix, self.local_temp_dir)
 
         expected = load_jsonl("tests/data/expected/dedupe-by-url.json.gz")
         computed = load_jsonl(f"{self.local_temp_dir}/tests/data/provided/attributes/dedupe_by_url/000.json.gz")
diff --git a/tests/python/test_mixer.py b/tests/python/test_mixer.py
index bd072a53..d4b81bca 100644
--- a/tests/python/test_mixer.py
+++ b/tests/python/test_mixer.py
@@ -10,6 +10,7 @@
     download_s3_prefix,
     get_test_prefix,
     load_jsonl,
+    skip_aws_tests,
     upload_s3_prefix,
 )
 
@@ -21,12 +22,18 @@
 
 class TestMixer(TestCase):
     def setUp(self) -> None:
-        self.test_prefix = get_test_prefix()
-        upload_s3_prefix(s3_prefix=f"{self.test_prefix}", local_prefix="tests/data/provided/**/*.gz")
-        upload_s3_prefix(s3_prefix=f"{self.test_prefix}", local_prefix="tests/data/provided/attributes/**/*.gz")
+        if skip_aws_tests():
+            self.remote_test_prefix = None
+        else:
+            self.remote_test_prefix = get_test_prefix()
+            upload_s3_prefix(s3_prefix=f"{self.remote_test_prefix}", local_prefix="tests/data/provided/**/*.gz")
+            upload_s3_prefix(
+                s3_prefix=f"{self.remote_test_prefix}", local_prefix="tests/data/provided/attributes/**/*.gz"
+            )
 
     def tearDown(self) -> None:
-        clean_test_data(self.test_prefix)
+        if self.remote_test_prefix is not None:
+            clean_test_data(self.remote_test_prefix)
 
     def test_email_spans(self):
         main(argv=["-c", str(EMAIL_SPANS), "mix"])
@@ -61,6 +68,9 @@ def test_paragraph_spans(self):
         )
 
     def test_local_input_remote_output(self):
+        if self.remote_test_prefix is None:
+            return self.skipTest("Skipping AWS tests")
+
         with open(MIXER, "r") as f:
             config = json.load(f)
 
@@ -68,7 +78,7 @@ def test_local_input_remote_output(self):
         local_output = config["streams"][0]["output"]["path"]
 
         # replace results path with s3 path
-        config["streams"][0]["output"]["path"] = f"{self.test_prefix}/{local_output}"
+        config["streams"][0]["output"]["path"] = f"{self.remote_test_prefix}/{local_output}"
 
         with NamedTemporaryFile("w") as f:
             json.dump(config, f)
@@ -76,7 +86,7 @@ def test_local_input_remote_output(self):
 
             main(argv=["-c", f.name, "mix"])
 
-        download_s3_prefix(f"{self.test_prefix}/tests/work", "tests/work/remote")
+        download_s3_prefix(f"{self.remote_test_prefix}/tests/work", "tests/work/remote")
 
         self.assertEqual(
             load_jsonl("tests/data/expected/mixer.json.gz"),
@@ -84,6 +94,9 @@ def test_local_input_remote_output(self):
         )
 
     def test_remote_input_remote_output(self):
+        if self.remote_test_prefix is None:
+            return self.skipTest("Skipping AWS tests")
+
         with open(MIXER, "r") as f:
             config = json.load(f)
 
@@ -92,10 +105,10 @@ def test_remote_input_remote_output(self):
         local_output = config["streams"][0]["output"]["path"]
 
         # replace results path with s3 path
-        config["streams"][0]["output"]["path"] = f"{self.test_prefix}/{local_output}"
+        config["streams"][0]["output"]["path"] = f"{self.remote_test_prefix}/{local_output}"
 
         # upload local input to s3, replace local input with s3 path
-        config["streams"][0]["documents"][0] = f"{self.test_prefix}/{local_input}"
+        config["streams"][0]["documents"][0] = f"{self.remote_test_prefix}/{local_input}"
 
         with NamedTemporaryFile("w") as f:
             json.dump(config, f)
@@ -103,7 +116,7 @@ def test_remote_input_remote_output(self):
 
             main(argv=["-c", f.name, "mix"])
 
-        download_s3_prefix(f"{self.test_prefix}/tests/work", "tests/work/remote")
+        download_s3_prefix(f"{self.remote_test_prefix}/tests/work", "tests/work/remote")
 
         self.assertEqual(
             load_jsonl("tests/data/expected/mixer.json.gz"),
@@ -111,6 +124,9 @@ def test_remote_input_remote_output(self):
         )
 
     def test_remote_input_local_output(self):
+        if self.remote_test_prefix is None:
+            return self.skipTest("Skipping AWS tests")
+
         with open(MIXER, "r") as f:
             config = json.load(f)
 
@@ -118,7 +134,7 @@ def test_remote_input_local_output(self):
         local_input = config["streams"][0]["documents"][0]
 
         # upload local input to s3, replace local input with s3 path
-        config["streams"][0]["documents"][0] = f"{self.test_prefix}/{local_input}"
+        config["streams"][0]["documents"][0] = f"{self.remote_test_prefix}/{local_input}"
 
         with NamedTemporaryFile("w") as f:
             json.dump(config, f)
diff --git a/tests/python/test_paths.py b/tests/python/test_paths.py
index 7318fb16..4f3518dd 100644
--- a/tests/python/test_paths.py
+++ b/tests/python/test_paths.py
@@ -18,18 +18,22 @@
     sub_suffix,
 )
 
-from .utils import clean_test_data, get_test_prefix, upload_s3_prefix
+from .utils import clean_test_data, get_test_prefix, skip_aws_tests, upload_s3_prefix
 
 LOCAL_DATA = Path(__file__).parent.parent / "data"
 
 
 class TestPaths(TestCase):
     def setUp(self) -> None:
-        self.test_prefix = get_test_prefix()
-        upload_s3_prefix(s3_prefix=f"{self.test_prefix}", local_prefix="tests/data/expected/*")
+        if skip_aws_tests():
+            self.remote_test_prefix = None
+        else:
+            self.remote_test_prefix = get_test_prefix()
+            upload_s3_prefix(s3_prefix=f"{self.remote_test_prefix}", local_prefix="tests/data/expected/*")
 
     def tearDown(self) -> None:
-        clean_test_data(self.test_prefix)
+        if self.remote_test_prefix is not None:
+            clean_test_data(self.remote_test_prefix)
 
     def test_pathify(self):
         path = "s3://path/to/file"
@@ -54,9 +58,12 @@ def test_local_glob_path(self):
         self.assertEqual(sorted(paths), sorted(expected))
 
     def test_remote_glob_path(self):
-        paths = list(glob_path(f"{self.test_prefix}/**/*.json.gz"))
+        if self.remote_test_prefix is None:
+            return self.skipTest("Skipping AWS tests")
+
+        paths = list(glob_path(f"{self.remote_test_prefix}/**/*.json.gz"))
         expected = [
-            f"{self.test_prefix}/tests/data/expected/{fn}"
+            f"{self.remote_test_prefix}/tests/data/expected/{fn}"
             for fn in os.listdir(LOCAL_DATA / "expected")
             if fn.endswith(".json.gz")
         ]
diff --git a/tests/python/utils.py b/tests/python/utils.py
index 6de87c9e..68cb6714 100644
--- a/tests/python/utils.py
+++ b/tests/python/utils.py
@@ -12,6 +12,7 @@
 from dolma.core.paths import glob_path, mkdir_p
 
 DOLMA_TEST_S3_PREFIX_ENV_VAR = "DOLMA_TESTS_S3_PREFIX"
+DOLMA_TESTS_SKIP_AWS_ENV_VAR = "DOLMA_TESTS_SKIP_AWS"
 DOLMA_TEST_S3_PREFIX_DEFAULT = "s3://dolma-tests"
 
 
@@ -45,15 +46,20 @@ def get_test_prefix() -> str:
     try:
         s3.head_bucket(Bucket=bucket)
     except Exception:
-        raise RuntimeError(
-            f"Unable to access test bucket '{test_prefix}'. To provide a different bucket, "
-            f"set the '{DOLMA_TEST_S3_PREFIX_ENV_VAR}' environment variable before running the tests."
-        )
+        if not skip_aws_tests():
+            raise RuntimeError(
+                f"Unable to access test bucket '{test_prefix}'. To provide a different bucket, "
+                f"set the '{DOLMA_TEST_S3_PREFIX_ENV_VAR}' environment variable before running the tests."
+            )
 
     # add a uuid to the test prefix to avoid collisions
     return f"{test_prefix.rstrip()}/{uuid.uuid4()}"
 
 
+def skip_aws_tests() -> bool:
+    return os.environ.get(DOLMA_TESTS_SKIP_AWS_ENV_VAR, "false").lower() == "true"
+
+
 def upload_test_documents(local_input: str, test_prefix: str) -> Tuple[str, str]:
     remote_input = f"{test_prefix}/input/documents"
     remote_output = f"{test_prefix}/output/documents"