Support checkpointing and reloading models #32

edwardjkim · 2019-09-09T04:39:31Z

Issue #, if available:

Description of changes:

Summary

Added callback.SaveCheckpoint: A callback for saving checkpoints.
Added train_utils.load_checkpoint: For reloading models upon resuming training.
Added unit tests in test/unit/algorithm_mode/test_callback.py.

Tests

Unit tests: pytest test/unit
Functional and integration tests: CR-12948549

Context

This PR is to support checkpointing for managed spot training, which was recently launched. Since spot instances can be interrupted at anytime, we need to be able to save checkpoints during training, and we also need to be able to resume from the last checkpoint when the training job is restarted.

To use managed spot training, customers specify EnableManagedSpotTraining and CheckpointConfig in CreateTrainingJob or the corresponding parameters in the SDK:

{
  "EnableManagedSpotTraining": true,
  "CheckpointConfig": { 
    "LocalPath": "/opt/ml/checkpoints",
    "S3Uri": "s3://bucket/key"
  }
}

When customers specify CheckpoiintConfig, the LocalPath becomes available in the config file at /opt/ml/input/config/checkpointconfig.json:

{
  "LocalPath": "/opt/ml/checkpoints"
}

Any files saved at this location will automatically be uploaded to S3. When training job is resumed from interruption, all uploaded files will be downloaded to the same location before training restarts.

Implementation

We first check if the config file is present at /opt/ml/input/config/checkpointconfig.json. If there is no config file, we don't save checkpoints. If it is available, we instantiate callback.SaveCheckpoint and add it to the list of callback functions that are called at the end of each iteration.

SaveCheckpoint saves each checkpoint to different files at the end of each iteration by appending the iteration number to the file name, e.g., xgboost-checkpont.1, xgboost-checkpont.2, and so on. When files are written to the directory specified by LocalPath, SM will automatically upload all files to the S3 location specified by S3Uri.

Since saving one checkpoint per iteration could result in a large number of files to save in S3 and download when spot instances are resumed, we retain only the 5 most recent checkpoints in the directory. This is accomplished by a background thread that deletes all checkpoints older than 5 most recent checkpoints (the number of files to keep is somewhat arbitrary, choosing the optimal number of files to keep is left for future work). Note that when a file is being uploaded by SageMaker, SM will create a marker file (file name + .sagemaker-uploading) to indicate that the file is being uploaded. SM will also create another marker file (file name + .sagemaker-uploaded) when the upload is completed. Thus, the background will skip deleting a file and try again later if there is a marker file <filename>.sagemaker-uploading and only attempt to delete a file when the marker file <filename>.sagemaker-uploaded is present.

CloudWatch logs

Suppose the customer is training for 100 rounds, and the spot instance is interrupted after iteration 49. When the training job is resumed, we want to load xgboost-checkpont.49 and restart training from iteration 50. We use the iteration numbers in the file names in our modified version of print_evaluation() function to provide logs that are consistent with what customers would expect.

First phase: Starting ↠ Downloading ↠ Training ↠ Interrupted

[17:41:02] 5845x9 matrix with 46760 entries loaded from /opt/ml/input/data/train
[17:41:02] 1252x9 matrix with 10016 entries loaded from /opt/ml/input/data/validation
INFO:root:Single node training.
INFO:root:Train matrix has 5845 rows
INFO:root:Validation matrix has 1252 rows
[0]#011train-rmse:8.11829#011validation-rmse:8.1177
...
[49]#011train-rmse:1.57065#011validation-rmse:1.91968

Second phase: Resumed ↠ Starting ↠ Downloading ↠ Training

[18:58:35] 5845x9 matrix with 46760 entries loaded from /opt/ml/input/data/train
[18:58:35] 1252x9 matrix with 10016 entries loaded from /opt/ml/input/data/validation
INFO:root:Single node training.
INFO:root:Checkpoint loaded from /opt/ml/checkpoints/xgboost-checkpoint.49
INFO:root:Resuming from iteration 50
[50]#011train-rmse:1.5635#011validation-rmse:1.91491
...
[99]#011train-rmse:1.30887#011validation-rmse:1.79031

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

aws-patlin · 2019-09-10T16:27:36Z

src/sagemaker_xgboost_container/algorithm_mode/train_utils.py

+import os
+import re
+from typing import Tuple, Optional
+import xgboost


For consistency, import xgboost as xgb (and update the corresponding references).

aws-patlin · 2019-09-10T16:34:51Z

src/sagemaker_xgboost_container/training.py


    """
    # TODO: replace with CSDK constants in sagemaker_containers._env
    train_config = json.load(open(os.getenv(sm_env_constants.SM_INPUT_TRAINING_CONFIG_FILE), "r"))
    data_config = json.load(open(os.getenv(sm_env_constants.SM_INPUT_DATA_CONFIG_FILE), "r"))

+    checkpoint_config_file = os.getenv(sm_env_constants.SM_CHECKPOINT_CONFIG_FILE)
+    if os.path.exists(checkpoint_config_file):
+        checkpoint_config = json.load(open(checkpoint_config_file, "r"))


From my understanding, it's not necessary to explicitly close an opened file, but this is generally considered bad practice. I notice that the same is done in the lines above (44-45). Thoughts?

I agree. I should have changed the previous lines instead of copying them. Changed.

aws-patlin · 2019-09-10T16:40:18Z

test/unit/algorithm_mode/test_callback.py

+
+        file_path = os.path.join(self.test_dir, "xgboost-checkpoint.000000000042")
+        self.assertTrue(os.path.isfile(file_path))
+        self.assertTrue(len(os.listdir(self.test_dir)), 1)


I'm assuming no file markers are generated in these unit tests, but do you check for/test them in the integration tests?

I have some basic unit tests for marker files in test_SaveCheckpoint_uploading. Integration tests don't really check for marker files.

Gotcha, it might be helpful for documentation to have some comments explaining what that test is doing, since it's not immediately clear to me what it's doing. I think the other tests are self-explanatory.

On that note, it would be good to include the information you've provided in this PR as comments in the code so it's clear for another developer in the future what you've done.

…-xgboost-container into checkpoint2

ericangelokim · 2019-09-11T15:15:43Z

src/sagemaker_xgboost_container/algorithm_mode/callback.py

+    def __init__(
+            self, checkpoint_dir: str, max_to_keep: int = 5, start_iteration: int = 0,
+            num_round: Optional[int] = None
+            ) -> None:


Ending parens should be in the same line as last element, see pep: https://www.python.org/dev/peps/pep-0008/#indentation

ericangelokim · 2019-09-11T15:16:29Z

src/sagemaker_xgboost_container/algorithm_mode/callback.py

+logging.basicConfig(level=logging.INFO)
+
+
+def _zero_pad(iteration: int) -> str:


Method names should ideally start with an action, i.e. '_add_zero_pad' unless it is a property

ericangelokim · 2019-09-11T15:19:56Z

src/sagemaker_xgboost_container/algorithm_mode/callback.py

+    return iter_num
+
+
+# modified from https://github.com/dmlc/xgboost/blob/master/python-package/xgboost/callback.py


Making sure the main change is the addition of the method argument start_iteration? Can you explain why this was required in comments?

Can we rename this method name to print_checkpointed_evaluation?

ericangelokim · 2019-09-11T15:24:43Z

src/sagemaker_xgboost_container/algorithm_mode/callback.py

+
+        return self.callback(env)
+
+    def fmt_path(self, i: int) -> str:


What is i? Why are we zero_padding the file name?

Also, if we are keeping this method, can you just rename it to format_path? You don't gain anything by removing the couple of letters, and you lose readability later.

Will change the variable and function names to be more descriptive.

On zero padding, I used the same format that tensorboard uses to save tfevent files (I just happened to be working on something else that uses tensorboard at the time). I think the rationale is by zero padding, it becomes string-sortable.

If that is the case, can you describe the rational for how much zero padding you want to do and document? Sortability is nice, but if you have a limited number of zero padding it affects scaleability. In general I would rather opt for scaleability rather than sortability; also why would users have to sort the checkpoints?

By scaleability, do you mean we might run out of zeros, like .999999999999 -> .000000000000. Choosing 12 digits was somewhat arbitrary, but I don't expect anyone to train for trillion rounds.

Sortability is useful in train_utils.load_checkpoint() where I sort the checkpoints and try to load the latest checkpoint, but that can easily be changed if we remove zero-padding. I think you have a point; from the user's perspective, zero-padding is unnecessary.

It's a valid point, so I could get rid of zero-padding and modify the sorting method in load_checkpoint(). Do you think that's better?

I agree with the trillion rounds, but no need to add limits that aren't necessary.

I think we should remove the zero-padding if there is no customer usecase for it.

ericangelokim · 2019-09-11T16:10:25Z

src/sagemaker_xgboost_container/algorithm_mode/callback.py

+    etc. However, this could result in a large number of files to save locally
+    (and on S3 if used in SageMaker). To save disk space and reduce the amount
+    of data required to be downloaded on resumption, we retain only the N
+    (default 5, spcified by 'max_to_keep') most recent checkpoints.


Based on my understanding, if we have checkpoints queued up, the upload would only happen on the most recent checkpoint. So we retain n - 1 most recent uploaded checkpoints the queued checkpoints that have not been uploaded yet.

If my understanding is incorrect please let me know.

ericangelokim · 2019-09-11T16:13:48Z

src/sagemaker_xgboost_container/algorithm_mode/callback.py

+            self, checkpoint_dir: str, max_to_keep: int = 5, start_iteration: int = 0,
+            num_round: Optional[int] = None
+            ) -> None:
+        """


The formatting of this file is different from the other files in the repo. Is this an artifact of the copy/paste of the print_evaluation method? I would prefer if copied methods are put in another file, and this file follow formatting patterns for cohesiveness.

ericangelokim · 2019-09-11T16:15:02Z

src/sagemaker_xgboost_container/algorithm_mode/callback.py

+
+
+class SaveCheckpoint:
+    """Create a callback that saves checkpoints to disk.


Minor: Doc reads like method documentation

ericangelokim · 2019-09-11T16:17:10Z

src/sagemaker_xgboost_container/algorithm_mode/callback.py

+    of data required to be downloaded on resumption, we retain only the N
+    (default 5, spcified by 'max_to_keep') most recent checkpoints.
+
+    To delete stale checkpoints, we use a producer-consumer pattern: we start a


This doc is nice, but we should also have comments near the code that actually executes it.

ericangelokim · 2019-09-11T19:15:16Z

src/sagemaker_xgboost_container/algorithm_mode/callback.py

+
+                path = self.fmt_path(i)
+                if (skip_locked_files
+                        and os.path.isfile(path + FILE_LOCK_SUFFIX)


Can you move these two .isfile() checks to a well named method for readability? I would also re-document the behavior here; whenever we have asynchronous processes I'd like to make sure its really clear what's happening.

ericangelokim · 2019-09-11T21:11:20Z

src/sagemaker_xgboost_container/algorithm_mode/callback.py

+        self.delete_queue.put(self.SENTINEL)
+        self.thread.join()
+
+    def callback(self, env: CallbackEnv) -> None:


Can you describe what is being done here? Between the file suffixes and trying to keep up with the file names its a bit confusing.

ericangelokim · 2019-09-11T21:14:53Z

src/sagemaker_xgboost_container/algorithm_mode/callback.py

+    return callback
+
+
+class SaveCheckpoint:


Overall this class seems much more complex than needed, or not complex enough. If we are deleting files based on 'flag' files why do we need to maintain a queue to remove files; wouldn't it be easier to just remove all files that we recognize as done?

On the other side, I'm not as familiar with spot instances; what exactly is the behavior when a spot instance is taken away? Does the kernel recognize it as a interrupt? Is this process supposed to block on uploading the files, or do we just take it as best attempt and take whatever was managed to be saved in s3?

We are constrained by how spot instances work and the way EASE uploads. When a spot instance is about to be taken away, we get an interruption notice via CloudWatch, and you get a two-minute window before the instance goes away. By the time the kernel gets a signal, it's too late; this will be after that two-minute warning expired and kernel is in the process of shutting down. EASE does not monitor this signal nor CW; it just keeps uploading the files until the instance shuts down, relying on the fact that all S3 uploads are all-or-nothing.

So, there is no guarantee that a callback will be executed inside that window before the instance is interrupted. We have to make our best attempt along the way without relying on the callback being called; hence the asynchronous deletion. And I'm seeing more stable performance with the async approach vs. deleting in the callback (simply removing files marked as done was actually my initial approach; sometimes I would see a very slow training here and there, probably by the callback being held up by deletion), although I will have to spend some time doing profiling and tests to back up that statement with a statistically significant result.

ericangelokim · 2019-09-12T15:12:58Z

src/sagemaker_xgboost_container/algorithm_mode/callback.py

@@ -0,0 +1,205 @@
+import logging


Can we rename this file to checkpointing.py? Let's be specific about the usecase of the code in the file.

OK, how about just checkpoint.py? Can I put pull it out one level up and put it as src/sagemaker_xgboost-container/checkpoint.py? I'm working on testing a script for the script mode, and this was intended to work both in script mode and algorithm mode.

Oh, I see serving.py, training.py, so checkpointing.py. Let's do checkpointing.py.

ericangelokim · 2019-09-12T18:05:39Z

src/sagemaker_xgboost_container/algorithm_mode/callback.py

+
+        os.rename(tf.name, self.fmt_path(i))
+
+        target_file = self.fmt_path(i - self.max_to_keep)


Can you explain the intution for this index please?

target_file is what I want to put on the queue. For example, if the current iteration is 5 (i.e., i == 5; I will use better variable names in the update), I want to put xgboost-checkpoint.0 on the queue, because I only want to keep xgboost-checkpoint.1 to .5. So when i == 5, i - self.max_to_keep == 0, and self.fmt_path(i - self.max_to_keep) returns /path/to/xgboost-checkpoint.0. Will update with better variable names, helper functions, and code comments.

ericangelokim

Looking better.

ericangelokim · 2019-09-16T19:46:31Z

src/sagemaker_xgboost_container/checkpointing.py

+    return callback
+
+
+def load_checkpoint(checkpoint_dir, max_try=5):


What are transient errors for an attempt to fail? Waiting for sagemaker to copy?

Training won't start until after all the checkpoints have been downloaded, so we are safe there. The retries are for checking if we have corrupted files (e.g., partial uploads from a previous run).

Why would we retry for corrupted files? those files will never successfully load right? Wouldn't it be better to fail fast?

ericangelokim · 2019-09-18T04:57:00Z

src/sagemaker_xgboost_container/checkpointing.py

+            # a delete signal to the agent so that the upload can be canceled and removed
+            # from S3.
+            self.delete_queue.put(self.SENTINEL)
+            _delete_once(skip_locked_files=False)


In the normal case, the second _delete_once doesn't so anything because the first call should only return once everything in the queue is removed right? otherwise won't it continue to loop while getting from queue?

This second _delete_once call is confusing to me

Yes, in the normal case, that function exits immediately and doesn't do anything.

I think my attempt to reuse the same function for two different things hurt readability here. I'm going to refactor and break _delete_once() into two functions even if they will share some code. I hope it improves readability.

To add, the reason I'm going through everything twice is to avoid potential file corruption when we delete a file that is still being uploaded, because the EASE team was concerned about this aspect when I talked to them. Again, the second call won't do anything in normal cases. Added code comments to address this point, and hopefully the refactor makes code easier to read.

ericangelokim · 2019-09-18T11:03:47Z

src/sagemaker_xgboost_container/checkpointing.py

+            uploaded = os.path.isfile(path + FILE_SAFE_SUFFIX)
+            return uploading and not uploaded
+
+        def _delete_once(skip_locked_files=True):


Why not just call this _delete_uploaded_files?

ericangelokim · 2019-09-18T11:05:13Z

src/sagemaker_xgboost_container/checkpointing.py

+        return checkpoint_path
+
+    def start(self):
+        """Starts a background thread that deletes old checkpoints


PEP 8 dictates that the docstring fgor methods read as a command, not a description:

"""Start background thread that ...

See more here: https://www.python.org/dev/peps/pep-0257/

I didn't know this. Good to know.

ericangelokim · 2019-09-18T11:07:35Z

src/sagemaker_xgboost_container/checkpointing.py

+        # (and on S3 if used in SageMaker). To save disk space and reduce the amount
+        # of data required to be downloaded on resumption, we retain only the N
+        # (default 5, spcified by 'max_to_keep') most recent checkpoints.
+        file_to_delete = self.format_path(current_iteration - self.max_to_keep)


I'm personally not a fan of huge inline block comments; if you think a piece of code requires this level of commenting to make sense, it usually means the code is too confusing. I would consider refactoring to make it more clear, or even moving to its own helper method for clarity.

ericangelokim · 2019-09-18T11:08:16Z

src/sagemaker_xgboost_container/checkpointing.py

+        # of data required to be downloaded on resumption, we retain only the N
+        # (default 5, spcified by 'max_to_keep') most recent checkpoints.
+        file_to_delete = self.format_path(current_iteration - self.max_to_keep)
+        file_to_delete_exists = os.path.isfile(file_to_delete)


Instead of checking this here, I would just check during the actual delete. For example, just because the file exists now doesn't necessarily mean it will exist at deletion time, so this check isn't very protective.

Thanks for this. I think this is a good suggestion.

ericangelokim · 2019-09-18T11:09:49Z

src/sagemaker_xgboost_container/checkpointing.py

+        offset_iteration = env.end_iteration if self.num_round is None else self.num_round
+        training_has_ended = (current_iteration + 1 >= self.start_iteration + offset_iteration)
+        if training_has_ended:
+            self.stop()


flake8 requires new line at end of file.

Is this true? When I include a new line, I get W391 blank line at end of file.

Have you run python3 -m tox

ericangelokim · 2019-09-18T11:11:57Z

src/sagemaker_xgboost_container/training.py

+    with open(os.getenv(sm_env_constants.SM_INPUT_DATA_CONFIG_FILE), "r") as f:
+        data_config = json.load(f)
+
+    checkpoint_config_file = os.getenv(sm_env_constants.SM_CHECKPOINT_CONFIG_FILE)


Very nitpick, but I would just follow the pattern established above and skip the variable initialization for checkpoint_config_file.

ericangelokim · 2019-09-18T21:19:46Z

src/sagemaker_xgboost_container/checkpointing.py

+    train_args["xgb_model"] = xgb_model
+    train_args["callbacks"] = callbacks
+    # xgboost's default value for num_boost_round is 10.
+    num_boost_round = train_args.get("num_boost_round", 10) - start_iteration


As a thought exercise; what if the default for xgb changes to something other than 10?

Should we try to expose this constant upstream? We could do a PR that adds something like DEFAULT_NUM_BOOST_ROUND = 10 in https://github.com/dmlc/xgboost/blob/master/python-package/xgboost/training.py and replaces all num_boost_round=10 with num_boost_round=DEFAULT_NUM_BOOST_ROUND.

ericangelokim · 2019-09-18T21:21:23Z

src/sagemaker_xgboost_container/checkpointing.py

+    # xgboost's default value for num_boost_round is 10.
+    num_boost_round = train_args.get("num_boost_round", 10) - start_iteration
+    # if last checkpoint is greater than num_boost_round, we shouldn't train.
+    train_args["num_boost_round"] = max(0, num_boost_round)


num_boost_round is negative, shouldn't we just return the model? you call train on the next line which seems unnecessary

We still need to call xgb.train() because xgb_model is a filename (str) while we have to return a booster object. But that line seems unnecessary. Removed it after testing xgb.train() with negative numbers and reading the source code upstream. Passing zero or negative number of rounds results in no training.

ericangelokim · 2019-09-18T21:35:05Z

src/sagemaker_xgboost_container/checkpointing.py

+    return booster
+
+
+# modified from https://github.com/dmlc/xgboost/blob/master/python-package/xgboost/callback.py


I'm not a fan of inline comments. Can we condense this information (lets not include log lines in docs) and put them in the method docs?

Actually, I realized this is the method you lifted. Let's condense the comments and not include log lines if possible!

ericangelokim · 2019-09-18T21:35:59Z

src/sagemaker_xgboost_container/checkpointing.py

+    return callback
+
+
+def load_checkpoint(checkpoint_dir, max_try=5):


Why would we retry for corrupted files? those files will never successfully load right? Wouldn't it be better to fail fast?

ericangelokim · 2019-09-19T03:05:36Z

src/sagemaker_xgboost_container/checkpointing.py

+        offset_iteration = env.end_iteration if self.num_round is None else self.num_round
+        training_has_ended = (current_iteration + 1 >= self.start_iteration + offset_iteration)
+        if training_has_ended:
+            self.stop()


Have you run python3 -m tox

ericangelokim · 2019-09-19T03:06:13Z

test/unit/test_checkpointing.py

+class TestSaveCheckpoint(unittest.TestCase):
+
+    def setUp(self):
+


Super nitpick and personal choice, but you have a lot of unneeded new lines in this file :)

On retrying, the thought was there's a chance that some files are intact even if others are corrupt. We retain N=5 checkpoints (N retires is basically why we save N checkpoints), and I think it's much cheaper for users to save N checkpoints and try them all than to train from scratch if 1 fails.

Ran python3 -m tox and confirmed that all tests/flake8 pass.

edwardjkim · 2019-09-19T17:51:24Z

Finished one final round of testing. Unit tests passed:

python3 -m tox

Required test coverage of 60% reached. Total coverage: 65.99%
py3-xgboost0.90: commands succeeded
flake8: commands succeeded
congratulations :)

Functional tests and integration tests passed. See CR-12948549.

I think it's ready to be merged.

Edward Kim added 2 commits September 8, 2019 21:26

Support checkpointing and reloading models

cca9c4c

Support checkpointing and reloading models

58b6c24

edwardjkim requested review from ericangelokim, aws-patlin, cbalioglu and iyerr3 September 9, 2019 19:13

edwardjkim marked this pull request as ready for review September 9, 2019 19:15

aws-patlin reviewed Sep 10, 2019

View reviewed changes

Edward Kim added 2 commits September 10, 2019 11:30

Merge branch 'checkpoint2' of https://github.com/edwardjkim/sagemaker…

931b06b

…-xgboost-container into checkpoint2

Minor fixes per comments

b8c61c0

edwardjkim requested a review from aws-patlin September 10, 2019 19:32

ericangelokim reviewed Sep 11, 2019

View reviewed changes

ericangelokim reviewed Sep 12, 2019

View reviewed changes

Refactor per comments in #32

55b4177

edwardjkim requested a review from ericangelokim September 16, 2019 16:45

Edward Kim added 2 commits September 17, 2019 02:56

Avoid modifying train_args in checkpointing.train

2ba0402

Add unit test for checkpointing.train

1e70da0

ericangelokim reviewed Sep 18, 2019

View reviewed changes

Refactor per comments in #32

a91655c

edwardjkim requested a review from ericangelokim September 18, 2019 20:07

Refactor per comments in #32

269d5df

ericangelokim reviewed Sep 19, 2019

View reviewed changes

Address comments in #32

7f84166

edwardjkim requested a review from ericangelokim September 19, 2019 06:01

Edward Kim added 2 commits September 19, 2019 00:19

Address comments in #32

5224eaf

Check return type of checkpointing.train

0b0fd68

edwardjkim merged commit c92642e into aws:master Sep 19, 2019

		logging.basicConfig(level=logging.INFO)


		def _zero_pad(iteration: int) -> str:

		return iter_num


		# modified from https://github.com/dmlc/xgboost/blob/master/python-package/xgboost/callback.py



		class SaveCheckpoint:
		"""Create a callback that saves checkpoints to disk.


		os.rename(tf.name, self.fmt_path(i))

		target_file = self.fmt_path(i - self.max_to_keep)

		return callback


		def load_checkpoint(checkpoint_dir, max_try=5):

		return booster


		# modified from https://github.com/dmlc/xgboost/blob/master/python-package/xgboost/callback.py

		class TestSaveCheckpoint(unittest.TestCase):

		def setUp(self):

Support checkpointing and reloading models #32

Support checkpointing and reloading models #32

Conversation

edwardjkim commented Sep 9, 2019 • edited

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

Choose a reason for hiding this comment

edwardjkim Sep 11, 2019 • edited

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

ericangelokim 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

ericangelokim Sep 18, 2019 • edited

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

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

edwardjkim commented Sep 19, 2019

edwardjkim commented Sep 9, 2019 •

edited

edwardjkim Sep 11, 2019 •

edited

ericangelokim Sep 18, 2019 •

edited