FileReader filter #3459

ksztenderski · 2021-10-28T16:49:40Z

Description

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactoring (Redesign of existing code that doesn't affect functionality)
Other (e.g. Documentation, Tests, Configuration)

What happened in this PR

It adds file_filter argument to FileReader allowing to filter files matching list of glob patterns.

Additional information

Affected modules and functionalities:
FileReader operator

Key points relevant for the review:

Checklist

Tests

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

JIRA TASK: N/A

review-notebook-app · 2021-10-28T16:53:22Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

mzient · 2021-10-28T21:06:13Z

Please rebase this PR on latest main.

ksztenderski · 2021-10-29T09:29:22Z

!build

dali-automaton · 2021-10-29T09:30:59Z

CI MESSAGE: [3291616]: BUILD STARTED

dali/operators/reader/loader/filesystem.cc

dali-automaton · 2021-10-29T12:41:24Z

CI MESSAGE: [3291616]: BUILD PASSED

jantonguirao · 2021-10-29T12:33:06Z

dali/benchmark/file_reader_bench.cc

@@ -0,0 +1,136 @@
+// Copyright (c) 2017-2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.


Suggested change

// Copyright (c) 2017-2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

// Copyright (c) 2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

jantonguirao · 2021-10-29T12:33:38Z

dali/benchmark/file_reader_bench.cc

+  // open the root
+  DIR *dir = opendir(file_root.c_str());
+
+  DALI_ENFORCE(dir != nullptr, "Directory " + file_root + " could not be opened.");


Suggested change

DALI_ENFORCE(dir != nullptr, "Directory " + file_root + " could not be opened.");

DALI_ENFORCE(dir != nullptr, make_string("Directory ", file_root, " could not be opened."));

nitpick

dali::make_string can be found in dali/core/format.h

jantonguirao · 2021-10-29T12:34:37Z

dali/operators/reader/file_reader_op.cc

@@ -41,7 +42,7 @@ For example, this directory structure::
  <file_root>/2/car.jpeg
  <file_root>/2/truck.jp2

-will yield the following outputs::
+without specified ``file_filters`` will yield the following outputs::


Suggested change

without specified ``file_filters`` will yield the following outputs::

by default will yield the following outputs::

jantonguirao · 2021-10-29T12:36:23Z

dali/operators/reader/loader/filesystem_test.cc

@@ -0,0 +1,124 @@
+// Copyright (c) 2017-2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.


Suggested change

// Copyright (c) 2017-2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

// Copyright (c) 2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

jantonguirao · 2021-10-29T12:48:09Z

dali/operators/reader/loader/filesystem.cc

+    if (HasKnownExtension(std::string(entry->d_name))) {
+      std::string rel_path = curr_entry + dir_sep + std::string{entry->d_name};
+      file_label_pairs.emplace_back(rel_path, label);


Suggested change

if (HasKnownExtension(std::string(entry->d_name))) {

std::string rel_path = curr_entry + dir_sep + std::string{entry->d_name};

file_label_pairs.emplace_back(rel_path, label);

std::string d_name(entry->d_name);

if (HasKnownExtension(d_name)) {

file_label_pairs.emplace_back(join_path(curr_entry, d_name), label);

nitpick: to avoid creating the same string twice

jantonguirao · 2021-10-29T13:15:32Z

dali/operators/reader/loader/filesystem_test.cc

+class FilesystemTest : public ::testing::Test {
+  std::vector<std::pair<std::string, int>> readFileLabelFile() {
+    std::vector<std::pair<std::string, int>> image_label_pairs;
+    std::string file_list = file_root + "/image_list.txt";
+    std::ifstream s(file_list);
+    DALI_ENFORCE(s.is_open(), "Cannot open: " + file_list);
+
+    std::vector<char> line_buf(16 << 10);
+    char *line = line_buf.data();
+    for (int n = 1; s.getline(line, line_buf.size()); n++) {
+      int i = strlen(line) - 1;
+
+      for (; i >= 0 && isspace(line[i]); i--) {}
+
+      int label_end = i + 1;
+
+      if (i < 0)
+        continue;
+
+      for (; i >= 0 && isdigit(line[i]); i--) {}
+
+      int label_start = i + 1;
+
+      for (; i >= 0 && isspace(line[i]); i--) {}
+
+      int name_end = i + 1;
+      DALI_ENFORCE(
+          name_end > 0 && name_end < label_start && label_start >= 2 && label_end > label_start,
+          make_string("Incorrect format of the list file \"", file_list, "\":", n,
+                      " expected file name followed by a label; got: ", line));
+
+      line[label_end] = 0;
+      line[name_end] = 0;
+
+      image_label_pairs.emplace_back(line, std::atoi(line + label_start));
+    }
+    std::sort(image_label_pairs.begin(), image_label_pairs.end());
+    DALI_ENFORCE(s.eof(), "Wrong format of file_list: " + file_list);
+
+    return image_label_pairs;
+  }
+
+ protected:
+  FilesystemTest()
+      : file_root(testing::dali_extra_path() + "/db/single/jpeg"),
+        file_label_pairs(readFileLabelFile()) {}
+
+  std::vector<std::string> globMatch(std::vector<std::string> &filters) {
+    std::vector<std::string> correct_match;
+    glob_t pglob;
+    for (auto &filter : filters) {
+      std::string pattern = file_root + filesystem::dir_sep + '*' + filesystem::dir_sep + filter;
+      if (glob(pattern.c_str(), GLOB_TILDE, NULL, &pglob) == 0) {
+        for (unsigned int count = 0; count < pglob.gl_pathc; ++count) {
+          std::string match(pglob.gl_pathv[count]);
+          correct_match.push_back(match.substr(file_root.length() + 1, std::string::npos));
+        }
+        globfree(&pglob);
+      }
+    }
+    std::sort(correct_match.begin(), correct_match.end());
+    std::unique(correct_match.begin(), correct_match.end());
+    return correct_match;
+  }


A thought. A significant part of this file is about re-implementing what the reader does, to build the reference. I think the test would be easier to read if we just hardcoded the input list of file/label and the expected outcome in each testcase.
I get that this makes it more flexible in case of changing the files in the test directory, but on the other hand it also introduces another source of bugs.
Feel free to disregard this idea if you feel is better as is now.

jantonguirao · 2021-10-29T13:17:18Z

dali/test/python/test_operator_readers_file.py

@@ -126,7 +128,35 @@ def test_file_reader_relpath_file_list():
            assert contents == ref_contents(fnames[index])


-batch_size_alias_test=64
+def _test_file_reader_filter(filters):
+    batch_size = 1


I'd test bigger batch size>1 as well

jantonguirao · 2021-10-29T13:17:26Z

dali/test/python/test_operator_readers_file.py

+def _test_file_reader_filter(filters):
+    batch_size = 1
+
+    pipe = Pipeline(batch_size, 1, 0)


I'd test with more than one thread

jantonguirao · 2021-10-29T13:19:08Z

dali/test/python/test_operator_readers_file.py

+        for filter in filters:
+            for file in glob.glob(os.path.join(root, dir, filter)):
+                fnames.append((label, file.split('/')[-1], file))


total nitpick: why not use a set() from the start?

jantonguirao · 2021-10-29T13:21:48Z

dali/benchmark/file_reader_bench.cc

+  DIR *dir = opendir(file_root.c_str());
+
+  DALI_ENFORCE(dir != nullptr, "Directory " + file_root + " could not be opened.");
+


Open question: Do we want those benchmarks to stay in DALI repository? Doesn't seem to be benchmarking any DALI component

It's to compare the two solutions.

klecki

Just some nitpick, I think we can go with making the list of the known extensions be the default - either add it in case of empty filters in traverse_directories or put it as the default there, and have the if in the call site. Probably the second one works better as the documentation.

klecki · 2021-10-29T08:47:56Z

dali/benchmark/file_reader_bench.cc

@@ -0,0 +1,135 @@
+// Copyright (c) 2017-2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.


Just a nitpick:

Suggested change

// Copyright (c) 2017-2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

// Copyright (c) 2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

klecki · 2021-10-29T14:31:12Z

dali/benchmark/file_reader_bench.cc

+  DIR *dir = opendir(file_root.c_str());
+
+  DALI_ENFORCE(dir != nullptr, "Directory " + file_root + " could not be opened.");
+


It's to compare the two solutions.

klecki · 2021-10-29T14:31:42Z

dali/benchmark/file_reader_bench.cc

+  // open the root
+  DIR *dir = opendir(file_root.c_str());
+
+  DALI_ENFORCE(dir != nullptr, "Directory " + file_root + " could not be opened.");


dali::make_string can be found in dali/core/format.h

Signed-off-by: ksztenderski <ksztenderski@nvidia.com>

…ngs argument. Signed-off-by: ksztenderski <ksztenderski@nvidia.com>

Signed-off-by: ksztenderski <ksztenderski@nvidia.com>

ksztenderski · 2021-11-02T13:25:51Z

!build

dali-automaton · 2021-11-02T13:31:12Z

CI MESSAGE: [3316146]: BUILD STARTED

klecki · 2021-11-02T13:42:42Z

dali/operators/reader/file_reader_op.cc

@@ -51,6 +53,13 @@ will yield the following outputs::
  <contents of 2/car.jpeg>          2
  <contents of 2/truck.jp2>         2

+and with ``file_filters = [*.jpg, *.jpeg]`` will yield the following outputs::


Nitpick:

Suggested change

and with ``file_filters = [*.jpg, *.jpeg]`` will yield the following outputs::

and with ``file_filters = ["*.jpg", "*.jpeg"]`` will yield the following outputs::

Still not addressed?

jantonguirao · 2021-11-02T14:09:26Z

dali/operators/reader/loader/utils.h

+static const std::vector<std::string> kKnownExtensionsGlob = {
+    "*.jpg", "*.jpeg", "*.png", "*.bmp",  "*.tif",  "*.tiff", "*.pnm", "*.ppm",
+    "*.pgm", "*.pbm",  "*.jp2", "*.webp", "*.flac", "*.ogg",  "*.wav"};
+


Have we checked how that displays on the documentation?

jantonguirao · 2021-11-02T14:09:56Z

dali/pipeline/operator/op_schema.cc

@@ -189,7 +189,7 @@ std::string OpSchema::GetArgumentDefaultValueString(const std::string &name) con

  auto &val = *value_ptr;
  auto str = val.ToString();
-  if (val.GetTypeId() == DALI_STRING ||
+  if (val.GetTypeId() == DALI_STRING || val.GetTypeId() == DALI_STRING_VEC ||


jantonguirao · 2021-11-02T14:11:06Z

dali/test/python/test_operator_readers_file.py

+
+def test_file_reader_filters():
+    for filters in [['*.jpg'], ['*.jpg', '*.png', '*.jpeg'], ['dog*.jpg', 'cat*.png', '*.jpg']]:
+        for num_threads in [1, 2, 4, 8]:


num_threads = random.choice([1, 2, 4, 8]) batch_size = random.choice([1, 3, 10])

should be enough, IMO

dali-automaton · 2021-11-02T14:48:59Z

CI MESSAGE: [3316146]: BUILD FAILED

Signed-off-by: ksztenderski <ksztenderski@nvidia.com>

jantonguirao · 2021-11-05T15:23:08Z

dali/operators/reader/file_reader_op.cc

@@ -29,7 +30,8 @@ This operator can be used in the following modes:

 In this mode, the directory indicated in ``file_root`` argument should contain one or more
 subdirectories. The files in these subdirectories are listed and assigned labels based on
-lexicographical order of the subdirectory.
+lexicographical order of the subdirectory. If you provide ``file_filters`` argument with
+list of glob strings, the operator will list files matching at least one of the patterns.


Suggested change

list of glob strings, the operator will list files matching at least one of the patterns.

a list of glob strings, the operator will list files matching at least one of the patterns. Otherwise, a default set of filters is used (see the default value of ``file_filters`` for details).

I'd also mention what happens by default

jantonguirao · 2021-11-05T15:23:36Z

dali/operators/reader/file_reader_op.cc

@@ -51,6 +53,13 @@ will yield the following outputs::
  <contents of 2/car.jpeg>          2
  <contents of 2/truck.jp2>         2

+and with ``file_filters = [*.jpg, *.jpeg]`` will yield the following outputs::


Still not addressed?

jantonguirao · 2021-11-05T15:26:11Z

dali/operators/reader/file_reader_op.cc

+  .AddOptionalArg<bool>("case_sensitive_filter", R"(If set to True, the filter will be applied
+to original file paths, otherwise to lower-cased file paths.)", false)


This is fine, but I think it'd be even better if we just make the filter case insensitive as well. That is, case insensitive filter="*.JPEG" would match "dog.jpeg"

jantonguirao · 2021-11-05T15:29:26Z

dali/operators/reader/loader/filesystem.cc

+    for (auto &filter : filters) {
+      if (fnmatch(filter.c_str(), entry->d_name, case_sensitive_filter ? 0 : FNM_CASEFOLD) == 0) {


Suggested change

for (auto &filter : filters) {

if (fnmatch(filter.c_str(), entry->d_name, case_sensitive_filter ? 0 : FNM_CASEFOLD) == 0) {

int flags = case_sensitive_filter ? 0 : FNM_CASEFOLD;

for (auto &filter : filters) {

if (fnmatch(filter.c_str(), entry->d_name, flags) == 0) {

According to the documentation it seems to me that the documentation is not accurate (the part saying that it matches lowercase filters).
I'd double check:

filter="*.JPEG" case_sensitive = false file="dog.jpeg"

^^ this should match. Then the documentation should be updated

jantonguirao · 2021-11-05T15:36:10Z

dali/pipeline/operator/op_schema.cc

@@ -189,6 +189,7 @@ std::string OpSchema::GetArgumentDefaultValueString(const std::string &name) con

  auto &val = *value_ptr;
  auto str = val.ToString();
+


revert? Otherwise this file would should changes from this PR, when there are actually none

Update FileReader documentation. Fix glob matching in nosetests for file_filters. Revert op_schema. Signed-off-by: ksztenderski <ksztenderski@nvidia.com>

ksztenderski · 2021-11-09T10:53:32Z

!build

dali-automaton · 2021-11-09T10:56:19Z

CI MESSAGE: [3361929]: BUILD STARTED

dali-automaton · 2021-11-09T13:12:33Z

CI MESSAGE: [3361929]: BUILD PASSED

Signed-off-by: ksztenderski <ksztenderski@nvidia.com>

ksztenderski · 2021-11-09T13:52:55Z

!build

dali-automaton · 2021-11-09T13:54:33Z

CI MESSAGE: [3362777]: BUILD STARTED

dali-automaton · 2021-11-09T13:59:59Z

CI MESSAGE: [3362799]: BUILD STARTED

dali-automaton · 2021-11-09T15:28:34Z

CI MESSAGE: [3362777]: BUILD PASSED

dali-automaton · 2021-11-09T16:11:37Z

CI MESSAGE: [3362799]: BUILD PASSED

Add file_filters argument to FileReader allowing to select files by matching glob patterns. Filters can be matched case-sensitively or case-insensitively by specifying the value of case_sensitive_filter argument (by default matching case-insensitive). Add benchmark comparing original matching by known extensions and the new matching with fnmatch. Changed implementation entirely to match only with fnmatch in (file, label) case. Add tests for file_filters in FileReader. Temporary fix for other readers that use FileLabelLoader and don't support case_sensitive_filter and thus do not assign it the default value when initializing with GetArgument("case_sensitive_filter"). Signed-off-by: ksztenderski <ksztenderski@nvidia.com>

ksztenderski requested a review from klecki October 28, 2021 16:49

ksztenderski self-assigned this Oct 28, 2021

ksztenderski force-pushed the file_reader_filter branch from 4e1e090 to 170d88b Compare October 28, 2021 16:53

klecki self-assigned this Oct 29, 2021

ksztenderski force-pushed the file_reader_filter branch 2 times, most recently from 3c55cdd to 175ad5c Compare October 29, 2021 08:34

klecki unassigned ksztenderski Oct 29, 2021

ksztenderski force-pushed the file_reader_filter branch from 53f8131 to b7532de Compare October 29, 2021 09:28

mzient reviewed Oct 29, 2021

View reviewed changes

dali/operators/reader/loader/filesystem.cc Outdated Show resolved Hide resolved

jantonguirao self-assigned this Oct 29, 2021

mzient reviewed Oct 29, 2021

View reviewed changes

dali/operators/reader/loader/filesystem.cc Outdated Show resolved Hide resolved

jantonguirao reviewed Oct 29, 2021

View reviewed changes

klecki reviewed Oct 29, 2021

View reviewed changes

ksztenderski added 5 commits November 2, 2021 14:18

file_filters argument for FileReader, benchmark and tests

f07572c

Signed-off-by: ksztenderski <ksztenderski@nvidia.com>

lint fix

1262b22

Signed-off-by: ksztenderski <ksztenderski@nvidia.com>

copyright fix

461e5b6

Signed-off-by: ksztenderski <ksztenderski@nvidia.com>

PR fixes, add default filters value, op_schema fix for vector of stri…

e99d0c0

…ngs argument. Signed-off-by: ksztenderski <ksztenderski@nvidia.com>

lint

1af339a

Signed-off-by: ksztenderski <ksztenderski@nvidia.com>

ksztenderski force-pushed the file_reader_filter branch from bae8436 to 1af339a Compare November 2, 2021 13:19

klecki approved these changes Nov 2, 2021

View reviewed changes

jantonguirao approved these changes Nov 2, 2021

View reviewed changes

add case_sensitive_filter argument

d401e9a

Signed-off-by: ksztenderski <ksztenderski@nvidia.com>

jantonguirao reviewed Nov 5, 2021

View reviewed changes

Update docs and nosetests

d1476d7

Update FileReader documentation. Fix glob matching in nosetests for file_filters. Revert op_schema. Signed-off-by: ksztenderski <ksztenderski@nvidia.com>

jantonguirao approved these changes Nov 9, 2021

View reviewed changes

klecki approved these changes Nov 9, 2021

View reviewed changes

Fix documentation

d374e2a

Signed-off-by: ksztenderski <ksztenderski@nvidia.com>

ksztenderski merged commit e04574d into NVIDIA:main Nov 9, 2021

		@@ -0,0 +1,136 @@
		// Copyright (c) 2017-2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

	// Copyright (c) 2017-2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
	// Copyright (c) 2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

	DALI_ENFORCE(dir != nullptr, "Directory " + file_root + " could not be opened.");
	DALI_ENFORCE(dir != nullptr, make_string("Directory ", file_root, " could not be opened."));

	without specified ``file_filters`` will yield the following outputs::
	by default will yield the following outputs::

		@@ -0,0 +1,124 @@
		// Copyright (c) 2017-2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

		DIR *dir = opendir(file_root.c_str());

		DALI_ENFORCE(dir != nullptr, "Directory " + file_root + " could not be opened.");

		@@ -0,0 +1,135 @@
		// Copyright (c) 2017-2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

	and with ``file_filters = [.jpg, .jpeg]`` will yield the following outputs::
	and with ``file_filters = [".jpg", ".jpeg"]`` will yield the following outputs::

	list of glob strings, the operator will list files matching at least one of the patterns.
	a list of glob strings, the operator will list files matching at least one of the patterns. Otherwise, a default set of filters is used (see the default value of ``file_filters`` for details).

		.AddOptionalArg<bool>("case_sensitive_filter", R"(If set to True, the filter will be applied
		to original file paths, otherwise to lower-cased file paths.)", false)

		for (auto &filter : filters) {
		if (fnmatch(filter.c_str(), entry->d_name, case_sensitive_filter ? 0 : FNM_CASEFOLD) == 0) {

		@@ -189,6 +189,7 @@ std::string OpSchema::GetArgumentDefaultValueString(const std::string &name) con

		auto &val = *value_ptr;
		auto str = val.ToString();

FileReader filter #3459

FileReader filter #3459

Conversation

ksztenderski commented Oct 28, 2021 • edited by klecki Loading

Description

What happened in this PR

Additional information

Checklist

Tests

Documentation

DALI team only

Requirements

review-notebook-app bot commented Oct 28, 2021

mzient commented Oct 28, 2021

ksztenderski commented Oct 29, 2021

dali-automaton commented Oct 29, 2021

dali-automaton commented Oct 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klecki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksztenderski commented Nov 2, 2021

dali-automaton commented Nov 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dali-automaton commented Nov 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksztenderski commented Nov 9, 2021

dali-automaton commented Nov 9, 2021

dali-automaton commented Nov 9, 2021

ksztenderski commented Nov 9, 2021

dali-automaton commented Nov 9, 2021

dali-automaton commented Nov 9, 2021

dali-automaton commented Nov 9, 2021

dali-automaton commented Nov 9, 2021

ksztenderski commented Oct 28, 2021 •

edited by klecki

Loading