Add AIHub and Modu Corpus (Especially local installed corpus) into lmdata task #194

hwiorn · 2021-01-20T02:23:58Z

Pull Request

Korpora에 기여해 주셔서 감사합니다.

해당 Pull Request를 제출하기 전에 아래 사항이 완료되었는지 확인 부탁드립니다:

작성한 코드가 어떤 에러나 경고 없이 실행이 되나요?
작성한 코드에 대한 테스트 코드를 만드셨나요? (경로 : Korpora/test)
기존 코드 역시 에러 없이 수행이 되겠죠?

1. 해당 PR은 어떤 내용인가요?

lmdata task에 AIHub 번역 데이터셋과 모두의 말뭉치 데이터셋의 학습 코퍼스 생성 기능 반영.
Local에 데이터셋 유무를 검사하는 코드를 Korpora.exists(..)로 추가.
테스트 코드 반영 및 exists 테스트 코드(manual.py) 반영

python manual.py exists --root-dir path/to/Korpora

2. PR과 관련된 이슈가 있나요?

#191

…data task

hwiorn · 2021-01-20T02:38:21Z

Korpora.exists 를 추가하면서, find_corpus_paths에 연관되어 중복되는 코드를 수정할 수 밖에 없었습니다. 최대한 기존 코드의 수정 부분을 줄이려고 했는데, 이 부분은 원저자의 의도와 다를 수 있으니, 검토가 필요할 듯 보입니다.

hwiorn · 2021-01-20T02:41:03Z

Korpora/task_lmdata.py

+    'modu_spoken': iterate_modu_spoken,
+    'modu_web': iterate_modu_web,
+    'modu_written': iterate_modu_written,
+    'aihub_spoken_translation': iterate_aihub_translation('aihub_spoken_translation'),


loader.py에서 KORPUS의 aihub_translation을 제공하여 전체 AIHUB 번역 데이터셋을 처리할 수 있으나, task_lmdata.py에서는 그렇게 처리할 경우 중복 데이터를 생성하게 되므로, ITERATE_TEXTS 구조에 따라 처리하기 위해, 불가피하게 aihub_translation 항목을 추가하지 않았습니다.

hwiorn · 2021-01-20T02:43:33Z

Korpora/utils.py

@@ -23,6 +23,23 @@ def check_dir(filepath):
        os.makedirs(dirname)


+def check_exists(corpus_name, informations, root_dir=None):


코퍼스의 local 존재 여부를 검사하는 코드가 각 코퍼스 클래스마다 중복되는 부분이 있어, check_exists 함수로 추가하였습니다.

압축파일을 다운받는 경우에는 information['destination'] 안에 압축파일의 local path 만 저장되어 있습니다. 만약 압축파일 다운로드 후, 압축해제만 실패한 경우, check_exists 함수에서는 해당 말뭉치가 설치된 것으로 나타나지만, 실제로는 말뭉치 파일이 없을 수 있습니다. 이는 #197 기능 구현 시 함께 엮어서 해결할 수 있을 듯 합니다.

지금 PR 에서는 압축파일 다운로드 후, 압축해제가 실패한 경우는 없다고 가정하고, 다음 PR 작업에서 이 부분도 해결할 수 있도록 하면 어떨까요?

네 맞습니다. 개인적으로 코퍼스를 검증하는 부분은 1. 작업파일이 정상적인지(md5 or sha1 검사)와 2. 파일이 제대로 풀렸는지(있는지) 등을 검사하면 될 것으로 생각합니다. 그렇기 때문에, 코퍼스가 제대로 있는지 확인하는 것은 추가 작업이라고 생각했고, 제 독단으로 한번의 PR로 반영하기에는 좀 부담스럽더군요(이번 PR의 범위를 넘어서는 부분)

다음 업데이트시에 반영하면 좋을 것 같습니다.

fetch 시에는 다운로드 받는 파일의 크기와 실제 local 에 있는 파일의 크기를 비교하는 방법을 이용하고 있습니다. 관련 코드 크기를 확인하는 것도 한 가지 방법일 듯 합니다. sha1 이나 md5 도 모두 좋습니다.

hwiorn · 2021-01-20T02:44:28Z

tests/manual.py

@@ -40,6 +40,13 @@ def fetch_test(args):
        time.sleep(0.5)


+def exists_test(args):


manual.py에 코퍼스 유무 검사를 위해 단순 test 기능을 추가하였습니다.

lovit

많은 양의 수정과 기능추가를 직접 해주셔서 감사합니다.

몇 가지 수정이 필요한 부분과, 수정을 제안하는 부분이 있습니다. 이 부분만 수정되면 merge 에 문제 없어보입니다.

또한, 본래 Korpora 의 작업은 master 에 배포 버전의 코드만 올라가고, 배포 전 개발 과정은 dev 에서 이뤄지고 있었습니다.

지난 번 PR 시 제가 이를 확인하지 않고 master 에 머징했었는데, 그러다보니 지금 master 와 dev 가 서로 sync 가 맞지 않습니다. 이번 PR 까지 master 에서 작업한 뒤, merge 하면서 dev 와 master 의 싱크를 맞춰두도록 하겠습니다.

혹시 이후에도 계속 PR 을 주신다면 dev 로 주시면 더 좋을 듯 합니다.

다시 한 번, 많은 양의 작업을 적극적으로 해주셔서 감사합니다.

lovit · 2021-01-24T09:17:16Z

Korpora/korpus_aihub_translation.py

        self.train = SentencePairKorpusData(
            f'{name}.train',
            *load_aihub_translation(paths, name)
        )

+    @classmethod
+    def get_corpus_path(cls, root_dir=None, prefix='', finder=None):


get_corpus_path 라는 classmethod 를 두는 것은 정말 좋은 방법이라고 생각합니다. AIHub translation corpus 외에도 압축파일로 다운로드를 하는 파일들의 경우, 압축 해제된 데이터 파일들의 path 를 관리하기가 어려웠는데, 이 classmethod 는 다른 클래스에서도 지원되면 좋을 기능 같아서 따로 #197 이슈로 넣어뒀습니다.

이 작업은 수정 내용이 많을 듯 하니 이 PR 이후에 다른 PR 로 작업하면 좋을 듯 합니다.

lovit · 2021-01-24T09:20:33Z

Korpora/korpus_aihub_translation.py

+        elif isinstance(root_dir, str) and os.path.isdir(root_dir):
+            root_dir = os.path.join(root_dir, 'AIHub_Translation', prefix)
+        paths = []
+        if finder:


아래의 paths = finder(root_dir) 을 살펴보면 finder 는 함수일 경우만 한정되는 듯 합니다. 안전하게 다음처럼 이 부분을 고치면 어떨까요?

if callable(finder): paths = finder(root_dir)

네 맞습니다. PR 업데이트하면서 반영하겠습니다.

lovit · 2021-01-24T09:27:22Z

Korpora/utils.py

@@ -23,6 +23,23 @@ def check_dir(filepath):
        os.makedirs(dirname)


+def check_exists(corpus_name, informations, root_dir=None):


압축파일을 다운받는 경우에는 information['destination'] 안에 압축파일의 local path 만 저장되어 있습니다. 만약 압축파일 다운로드 후, 압축해제만 실패한 경우, check_exists 함수에서는 해당 말뭉치가 설치된 것으로 나타나지만, 실제로는 말뭉치 파일이 없을 수 있습니다. 이는 #197 기능 구현 시 함께 엮어서 해결할 수 있을 듯 합니다.

지금 PR 에서는 압축파일 다운로드 후, 압축해제가 실패한 경우는 없다고 가정하고, 다음 PR 작업에서 이 부분도 해결할 수 있도록 하면 어떨까요?

lovit · 2021-01-24T09:29:24Z

Korpora/korpus_kcbert.py

@@ -67,6 +67,10 @@ def __init__(self, root_dir=None, force_download=False):
            dirname = os.path.abspath(f'{root_dir}/kcbert')
            self.train = f'KcBERT corpus is downloaded. Open local directory {dirname}'

+    @classmethod
+    def exists(cls, root_dir=None):
+        return check_exists('korean_kcbert', KCBERT_FETCH_INFORMATION, root_dir=root_dir)


kcbert 코퍼스는 korean_kcbert 가 아닌, kcbert 라는 이름을 이용하고 있습니다. 이 부분은 수정이 필요해 보입니다.

또한, 본래 Korpora 의 작업은 master 에 배포 버전의 코드만 올라가고, 배포 전 개발 과정은 dev 에서 이뤄지고 있었습니다.

지난 번 PR 시 제가 이를 확인하지 않고 master 에 머징했었는데, 그러다보니 지금 master 와 dev 가 서로 sync 가 맞지 않습니다. 이번 PR 까지 master 에서 작업한 뒤, merge 하면서 dev 와 master 의 싱크를 맞춰두도록 하겠습니다.

혹시 이후에도 계속 PR 을 주신다면 dev 로 주시면 더 좋을 듯 합니다.

앞으로 dev 브랜치로 PR 반영하겠습니다 :)

lovit · 2021-01-24T09:32:50Z

Korpora/korpus_kowiki.py

@@ -82,6 +82,10 @@ def split_title_text(wikitext):
        # swap position
        return texts, titles

+    @classmethod
+    def exists(cls, root_dir=None):
+        return check_exists('kowiki', KOWIKI_FETCH_INFORMATION, root_dir=root_dir)


한국어 위키피디아 텍스트 데이터셋은 kowiki 가 아닌 kowikitext 라는 이름을 쓰고 있습니다. 이 부분 수정이 필요해 보입니다.

더하여 KOWIKI_FETCH_INFORMATION 때문에 이름이 혼란스러울 수 있으므로, 현재 PR 혹은 다음 작업 시 KOWIKI_FETCH_INFORMATION 도 KOWIKITEXT_FETCH_INFORMATION 로 이름을 수정하도록 하겠습니다.

각 코퍼스 클래스마다 동일한 문자열을 중복해서 쓰고 있는데 license, description처럼 corpus_name 같은 상수를 정의하고, local path와 check_exists등에 가져다 쓰는건 혹시 어떠신가요? 개인적으로는 loader.py에 들어가있는 structure들의 키 값을 각 코퍼스 모듈의 corpus_name을 가져다 쓰는 것도 괜찮지 않나 싶습니다.

말씀하신 구조가 대략 아래와 같은 예시일까요?

class KcBERTKorpus(): corpus_name = "kcbert" fetch_information = [ { 'url': 'https://github.com/Beomi/KcBERT/releases/download/TrainData_v1/kcbert-train.tar.gzaa', 'destination': 'kcbert/kcbert-train.tar.gzaa', 'method': 'download' }, { 'url': 'https://github.com/Beomi/KcBERT/releases/download/TrainData_v1/kcbert-train.tar.gzab', 'destination': 'kcbert/kcbert-train.tar.gzab', 'method': 'download' }, { 'url': 'https://github.com/Beomi/KcBERT/releases/download/TrainData_v1/kcbert-train.tar.gzac', 'destination': 'kcbert/kcbert-train.tar.gzac', 'method': 'download' } ] @classmethod def check_exists(cls, root_dir=None): # ... return None @classmethod def get_corpus_path(cls, root_dir=None): if root_dir is None: root_dir = default_korpora_root_dir corpus_paths = [] for information in KcBERTKorpus.fetch_information: corpus_paths.append(f"{root_dir}/{information['destination']}") return corpus_paths KcBERTKorpus.corpus_name KcBERTKorpus.fetch_information

네, 맞습니다

위의 구조면 fetch_information 에 data size 나 sha1 값을 넣어두고 데이터 정합성도 확인할 수 있을듯 하니 정말 좋은 구조라 생각합니다.
좋은 제안 해주셔서 감사합니다.

이 내용도 사이즈가 큰 작업이니 독립된 PR 로 작업하면 좋을 듯 합니다.

lovit · 2021-01-24T09:34:21Z

Korpora/korpus_modu_messenger.py

+MODU_MESSENGER_INFORMATION = [
+    {
+        'url': 'https://github.com/lovit/kowikitext/releases/download/kowikitext.20200920.v2/kowikitext_20200920.train.zip',
+        'destination': 'kowikitext/kowikitext_20200920.train.zip',
+        'method': None,
+    }
+]


모두의 말뭉치에 kowikitext 정보가 추가되었습니다. 제거가 필요해 보입니다.

lovit · 2021-01-24T09:47:33Z

Korpora/loader.py

+            root_dir = default_korpora_path
+
+        corpora = [KORPUS[name].exists(root_dir=root_dir) for name in corpus_name]
+        if return_all:


return_all 은 corpus_name 에 입력된 각각의 말뭉치마다 exists 의 유무를 확인하겠다는 의도로 이해했습니다. 이를 명시하는 argument name 으로 return_all 보다는 return_by_each_corpus 같은 이름이 좀 더 직관적이지 않을까 제안드립니다.

네, Korpus.exists(...)의 시그니쳐가 코퍼스 리스트를 받아들이기 때문에, 코퍼스 리스트의 결과를 받는 것이 좀더 유연하게 사용할 수 있을 것 같아 추가하였습니다. 매개변수 이름을 말씀하신대로 업데이트 하겠습니다.

lovit

이번 PR 범위에서 수정할 내용들은 모두 수정되었음을 확인했습니다. 추가 작업이 없으시다면 merge 하겠습니다.

우선 approve 하였습니다. 적극적으로 기여해주셔서 감사합니다.

lovit · 2021-01-27T20:23:43Z

@hwiorn 보내주신 PR 내용, dev branch 에 rebase 완료하였습니다.

Add AIHub and Modu Corpus (Especially local installed corpus) into lm…

1359c3c

…data task

hwiorn commented Jan 20, 2021

View reviewed changes

lovit requested review from lovit and ratsgo and removed request for lovit January 20, 2021 04:39

lovit requested changes Jan 24, 2021

View reviewed changes

Fix some requested changes

f8784ea

lovit approved these changes Jan 27, 2021

View reviewed changes

lovit merged commit 2a2be9d into ko-nlp:master Jan 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AIHub and Modu Corpus (Especially local installed corpus) into lmdata task #194

Add AIHub and Modu Corpus (Especially local installed corpus) into lmdata task #194

hwiorn commented Jan 20, 2021

hwiorn commented Jan 20, 2021

hwiorn Jan 20, 2021

hwiorn Jan 20, 2021

lovit Jan 24, 2021

hwiorn Jan 26, 2021

lovit Jan 26, 2021

hwiorn Jan 20, 2021 •

edited

Loading

lovit left a comment

lovit Jan 24, 2021

lovit Jan 24, 2021

hwiorn Jan 26, 2021

lovit Jan 24, 2021

lovit Jan 24, 2021

hwiorn Jan 26, 2021

lovit Jan 24, 2021

hwiorn Jan 27, 2021

lovit Jan 27, 2021

hwiorn Jan 27, 2021

lovit Jan 27, 2021

lovit Jan 24, 2021

lovit Jan 24, 2021

hwiorn Jan 26, 2021

lovit left a comment

lovit commented Jan 27, 2021

		@@ -23,6 +23,23 @@ def check_dir(filepath):
		os.makedirs(dirname)


		def check_exists(corpus_name, informations, root_dir=None):

		@@ -40,6 +40,13 @@ def fetch_test(args):
		time.sleep(0.5)


		def exists_test(args):

Add AIHub and Modu Corpus (Especially local installed corpus) into lmdata task #194

Add AIHub and Modu Corpus (Especially local installed corpus) into lmdata task #194

Conversation

hwiorn commented Jan 20, 2021

Pull Request

1. 해당 PR은 어떤 내용인가요?

2. PR과 관련된 이슈가 있나요?

hwiorn commented Jan 20, 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

hwiorn Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

lovit 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

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

lovit left a comment

Choose a reason for hiding this comment

lovit commented Jan 27, 2021

hwiorn Jan 20, 2021 •

edited

Loading