Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions config_root/config/config_list_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def get_universe_top_n(universe: List[Any], n: Optional[int]) -> List[Any]:
# #############################################################################


# TODO(gp): -> get_time_interval
# TODO(gp): GFI -> get_time_interval
def get_period(period: str) -> Tuple[pd.Timestamp, pd.Timestamp]:
Comment on lines +66 to 67
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is used in other repos too (cmamp, orange)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I'll revert and come up with a system to check across the repos.

"""
Get start and end timestamps from the specified period.
Expand Down Expand Up @@ -193,11 +193,11 @@ def build_config_list_varying_asset_id(
# #############################################################################


# TODO(gp): -> ...varying_asset_tiles
# TODO(gp): GFI -> ...varying_asset_tiles
def build_config_list_varying_universe_tiles(
Comment on lines +196 to 197
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also used in cmamp

config_list: cconfig.ConfigList,
universe_tile_id: cconfig.CompoundKey,
# TODO(gp): -> asset_tiles
# TODO(gp): GFI -> asset_tiles
universe_tiles: List[List[int]],
) -> cconfig.ConfigList:
"""
Expand Down Expand Up @@ -226,7 +226,7 @@ def build_config_list_varying_universe_tiles(
return config_list_out


# TODO(gp): -> ...varying_period_tiles
# TODO(gp): GFI -> ...varying_period_tiles
def build_config_list_varying_tiled_periods(
Comment on lines +229 to 230
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used in cmamp

config_list: cconfig.ConfigList,
start_timestamp: pd.Timestamp,
Expand Down Expand Up @@ -255,7 +255,7 @@ def build_config_list_varying_tiled_periods(
hdateti.dassert_has_tz(start_timestamp)
hdateti.dassert_has_tz(end_timestamp)
hdbg.dassert_lte(start_timestamp, end_timestamp)
# TODO(gp): Check that the lookback is > 0.
# TODO(gp): GFI Check that the lookback is > 0.
lookback = pd.Timedelta(lookback_as_pd_str)
#
configs = []
Expand Down Expand Up @@ -311,7 +311,7 @@ def build_config_list_varying_tiled_periods(
# #############################################################################


# TODO(gp): -> build_config_list_using_equal_asset_tiles
# TODO(gp): GFI -> build_config_list_using_equal_asset_tiles
def build_config_list_with_tiled_universe(
Comment on lines +314 to 315
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used in cmamp

config_list: cconfig.ConfigList, asset_ids: List[int]
) -> cconfig.ConfigList:
Expand Down Expand Up @@ -367,7 +367,7 @@ def apply_build_config_list(
return config_list_out


# TODO(gp): -> build_config_list_using_equal_asset_and_period_tiles
# TODO(gp): GFI -> build_config_list_using_equal_asset_and_period_tiles
def build_config_list_with_tiled_universe_and_periods(
Comment on lines +370 to 371
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used in cmamp

config_list: cconfig.ConfigList,
) -> cconfig.ConfigList:
Expand Down
6 changes: 3 additions & 3 deletions config_root/config/config_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ def replace_shared_dir_paths(
return new_config


# TODO(gp): Add unit tests.
# TODO(gp): GSI: Add unit tests.
def sort_config_string(txt: str) -> str:
"""
Sort a string representing a Config in alphabetical order by the first
Expand Down Expand Up @@ -475,7 +475,7 @@ def diff_configs(configs: Iterable[crococon.Config]) -> List[crococon.Config]:
# #############################################################################


# TODO(gp): Is this private?
# TODO(gp): GFI: Is this private?
def convert_to_series(config: crococon.Config) -> pd.Series:
"""
Convert a config into a flattened series representation.
Expand All @@ -501,7 +501,7 @@ def convert_to_series(config: crococon.Config) -> pd.Series:
return srs


# TODO(gp): Is this private?
# TODO(gp): GFI: Is this private?
def convert_to_dataframe(configs: Iterable[crococon.Config]) -> pd.DataFrame:
"""
Convert multiple configs into flattened dataframe representation.
Expand Down
4 changes: 2 additions & 2 deletions config_root/config/test/test_config_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def _build_test_config_list(
# #############################################################################


# TODO(gp): -> Test_get_config_list_from_builder1
# TODO(gp): GFI -> Test_get_config_list_from_builder1
class TestGetConfigsFromBuilder1(hunitest.TestCase):
def test1(self) -> None:
"""
Expand All @@ -49,7 +49,7 @@ def test1(self) -> None:
# #############################################################################


# TODO(gp): -> Test_get_config_from_env1
# TODO(gp): GFI -> Test_get_config_from_env1
class TestGetConfigFromEnv(hunitest.TestCase):
def test_no_env_variables(self) -> None:
"""
Expand Down
2 changes: 1 addition & 1 deletion config_root/config/test/test_config_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def _get_test_config4() -> cconfig.Config:
# #############################################################################


# TODO(gp): -> validate_config_list
# TODO(gp): GFI -> validate_config_list?
class Test_validate_configs1(hunitest.TestCase):

def test_check_same_configs_error(self) -> None:
Expand Down
1 change: 0 additions & 1 deletion helpers/henv.py
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,6 @@ def _git_log(num_commits: int = 5, my_commits: bool = False) -> str:
if my_commits:
# This doesn't work in a container if the user relies on `~/.gitconfig` to
# set the user name.
# TODO(gp): We should use `get_git_name()`.
cmd.append("--author $(git config user.name)")
cmd = " ".join(cmd)
data: Tuple[int, str] = hsystem.system_to_string(cmd)
Expand Down
2 changes: 1 addition & 1 deletion helpers/hio.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def listdir(
cmd.append(r'-not -path "*/\.git/*"')
cmd = " ".join(cmd)
_, output = hsystem.system_to_string(cmd)
# TODO(gp): -> system_to_files
# TODO(gp): GSI -> system_to_files
paths = [path for path in output.split("\n") if path != ""]
_LOG.debug("Found %s paths in %s", len(paths), dir_name)
_LOG.debug("\n".join(paths))
Expand Down
3 changes: 0 additions & 3 deletions helpers/hlatex.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@
import helpers.hio as hio
import helpers.hprint as hprint

# TODO(gp): Consider using `pypandoc` instead of calling `pandoc` directly.
# https://boisgera.github.io/pandoc


# TODO(gp): Add a switch to keep the tmp files or delete them.
def convert_pandoc_md_to_latex(txt: str) -> str:
Expand Down
4 changes: 2 additions & 2 deletions helpers/hobject.py
Original file line number Diff line number Diff line change
Expand Up @@ -472,8 +472,8 @@ def to_config_str(self) -> str:
# #############################################################################


# TODO(gp): CleanUp. This is for testing and should be in hobject_test.py.
# TODO(gp): -> check_object_signature
# TODO(gp): GSI. This is for testing and should be in hobject_test.py.
# TODO(gp): GSI -> check_object_signature
def test_object_signature(
Comment on lines +475 to 477
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used in cmamp

self_: Any, obj: Any, *, remove_lines_regex: Optional[str] = None
) -> None:
Expand Down
10 changes: 5 additions & 5 deletions helpers/hpandas.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ def dassert_unique_index(
hdbg.dassert(index.is_unique, msg=msg, *args)


# TODO(gp): @all Add unit tests.
# TODO(gp): GSI. Add unit tests.
def dassert_increasing_index(
Comment on lines +144 to 145
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests already exist:

class Test_dassert_increasing_index(hunitest.TestCase):

Either an outdated TODO, or need more specs on what tests still have to be added.

obj: Union[pd.Index, pd.DataFrame, pd.Series],
msg: Optional[str] = None,
Expand Down Expand Up @@ -181,7 +181,7 @@ def dassert_increasing_index(
hdbg.dassert(index.is_monotonic_increasing, msg=msg, *args)


# TODO(gp): @all Add more info in case of failures and unit tests.
# TODO(gp): GSI. Add more info in case of failures and unit tests.
def dassert_strictly_increasing_index(
Comment on lines +184 to 185
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, tests already exist:

class Test_dassert_strictly_increasing_index(hunitest.TestCase):

Can probably change the TODO to only "Add more info in case of failures", if that's still relevant.

obj: Union[pd.Index, pd.DataFrame, pd.Series],
msg: Optional[str] = None,
Expand Down Expand Up @@ -210,7 +210,7 @@ def dassert_monotonic_index(
hdbg.dassert(cond, msg=msg, *args)


# TODO(Paul): @gp -> dassert_datetime_indexed_df
# TODO(Paul): GSI -> dassert_datetime_indexed_df
def dassert_time_indexed_df(
Comment on lines +213 to 214
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used in cmamp

df: pd.DataFrame, allow_empty: bool, strictly_increasing: bool
) -> None:
Expand Down Expand Up @@ -1776,11 +1776,11 @@ def get_random_df(

# #############################################################################

# TODO(gp): -> AxisNameSet
# TODO(gp): GSI -> AxisNameSet
ColumnSet = Optional[Union[str, List[str]]]


# TODO(gp): -> _resolve_axis_names
# TODO(gp): GSI -> _resolve_axis_names
def _resolve_column_names(
column_set: ColumnSet,
columns: Union[List[str], pd.Index],
Expand Down
8 changes: 4 additions & 4 deletions helpers/lib_tasks_docker.py
Original file line number Diff line number Diff line change
Expand Up @@ -675,7 +675,7 @@ def _generate_docker_compose_file(
],
"extends": "app",
"network_mode": "${NETWORK_MODE:-bridge}",
# TODO(gp): Rename `AM_PORT`.
# TODO(gp): Rename `CSFY_PORT`.
"ports": [
"${PORT}:${PORT}",
],
Expand Down Expand Up @@ -981,7 +981,7 @@ def _get_base_image(base_image: str) -> str:
:return: e.g., *****.dkr.ecr.us-east-1.amazonaws.com/amp
"""
if base_image == "":
# TODO(gp): Use os.path.join.
# TODO(gp): GFI. Use os.path.join.
base_image = (
hlitauti.get_default_param("CSFY_ECR_BASE_PATH")
+ "/"
Expand All @@ -1008,8 +1008,8 @@ def _get_base_image(base_image: str) -> str:
# return tag_name


# TODO(gp): Consider using a token "latest" in version, so that it's always a
# string and we avoid a special behavior encoded in None.
# TODO(gp): GSI: Consider using a token "latest" in version, so that it's
# always a string and we avoid a special behavior encoded in None.
def get_image(
base_image: str,
stage: str,
Expand Down
3 changes: 0 additions & 3 deletions helpers/test/test_hcache.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@

_LOG = logging.getLogger(__name__)

# TODO(gp): Do not commit this.
# _LOG.debug = _LOG.info


# TODO(gp): Use hprint.log_frame
def _LOG_frame(txt: str) -> None:
Expand Down
4 changes: 2 additions & 2 deletions helpers/test/test_hdbg.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@
# #############################################################################


# TODO(gp): Use a self.assert_equal() instead of a check_string() since this
# code needs to be stable.
# TODO(gp): GSI: Use a self.assert_equal() instead of a check_string() since
# this code needs to be stable.
class Test_dassert1(hunitest.TestCase):
"""
Test `dassert()`.
Expand Down
111 changes: 0 additions & 111 deletions linters/test/test_amp_dev_scripts.py
Original file line number Diff line number Diff line change
Expand Up @@ -396,114 +396,3 @@ def test1(self) -> None:
# Comment before initializing.
class TestAnother():
pass

if __name__ == "main":
Comment on lines -399 to -400
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is all of that deleted? By mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looked weird (and without comment explaining it) so I thought it was a leftover. pytest collects the tests and we don't use the entrypoint.

Should it be a manual test? (I.e., a disabled pytest?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deleted content is

  • the last part of the function _get_horrible_python_code1(), which provides code for two end-to-end Linter tests
  • the whole of the function _get_ipynb_contents1(), which provides code for the test of linting a notebook
  • the whole of the function _run_linter(), which actually runs Linter for the tests

It all should be restored, otherwise all the tests will break.
If anything, we should add a TODO to move the code from _get_horrible_python_code1() and _get_ipynb_contents1() to input dirs

txt = "hello"
m = re.search("\s", txt)
n = nltk.word_tokenize(txt)
hdbg.dassert_path_exists("filename.txt")
'''
return txt

@staticmethod
def _get_ipynb_contents1() -> str:
contents_ipynb = r"""
{
"cells": [
{
"cell_type": "markdown",
"metadata": {},
"source": [
"# Imports"
]
},
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": [
"import pandas as pd\n",
"import re\n",
"\n",
"# TODO: Fix.\n",
"res = re.findall(r\"[a-z]+\", \"some text\")\n",
"\n",
"\n"
]
}
],
"metadata": {
"kernelspec": {
"display_name": "Python 3 (ipykernel)",
"language": "python",
"name": "python3"
}
},
"nbformat": 4,
"nbformat_minor": 5
}
"""
return contents_ipynb

def _run_linter(
self,
file_name: str,
linter_log: str,
as_system_call: bool,
) -> str:
if as_system_call:
cmd = []
cmd.append(f"linters/base.py --files {file_name}")
cmd_as_str = " ".join(cmd)
## We need to ignore the errors reported by the script, since it
## represents how many lints were found.
suppress_output = _LOG.getEffectiveLevel() > logging.DEBUG
hsystem.system(
cmd_as_str,
abort_on_error=False,
suppress_output=suppress_output,
output_file=linter_log,
)
cmd_rm = f"git restore --staged {file_name}"
hsystem.system(cmd_rm, abort_on_error=False)
else:
logger_verbosity = hdbg.get_logger_verbosity()
parser = libase._parse()
args = parser.parse_args(
[
"-f",
file_name,
"--linter_log",
linter_log,
# TODO(gp): Avoid to call the logger.
"-v",
"ERROR",
]
)
libase._main(args)
hdbg.init_logger(logger_verbosity)

# Read log.
_LOG.debug("linter_log=%s", linter_log)
txt = hio.from_file(linter_log)
# Process log.
output = []
output.append("# linter log")
for line in txt.split("\n"):
# Remove the line:
# Cmd line='.../linter.py -f input.py --linter_log ./linter.log'
if "cmd line=" in line:
continue
# Filter out code rate because of #2241.
if "Your code has been rated at" in line:
continue
output.append(line)
# Read output.
_LOG.debug("file_name=%s", file_name)
output.append("# linter file")
txt = hio.from_file(file_name)
output.extend(txt.split("\n"))
# //////////////
output_as_str = "\n".join(output)
return output_as_str
8 changes: 4 additions & 4 deletions linters/test/test_amp_fix_md_links.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ def write_input_file(self, txt: str, file_name: str) -> str:
hio.to_file(file_path, txt)
return file_path

# TODO(gp): To outsource. Break into smaller tests. If one of these fails,
# it's hard to debug.
# TODO(gp): GSI. Break into smaller tests. If one of these fails, it's hard
# to debug.
def test1(self) -> None:
"""
Test fixing link formatting in a Markdown file.
Expand Down Expand Up @@ -205,8 +205,8 @@ def test2(self) -> None:
output = _get_output_string(out_warnings, updated_lines)
self.check_string(output, purify_text=True)

# TODO(gp): To outsource. Break into smaller tests. If one of these fails,
# it's hard to debug.
# TODO(gp): GSI. Break into smaller tests. If one of these fails, it's hard
# to debug.
def test3(self) -> None:
"""
Test the mix of Markdown and HTML-style links.
Expand Down