Review PR#70
Conversation
| return | ||
| str_to_write = task[0] + "," + task[1] + "\n" | ||
| with open(result_file, "a") as f: | ||
| f.write(str_to_write) |
There was a problem hiding this comment.
m7 func2 worker never exits and join() can hang forever
func2 doesn't look right.
Queue.get() blocks when the queue is empty; it does not raise queue.Empty. except task_q.empty is also not a valid exception handler (TypeError if anything else is raised in try). Workers that finish their items will block forever on the next get(). task_done() is never called, so tasks2.join() can wait indefinitely.
Steps to reproduce:
cd practice/m7_concurrency/task1_fibonacci
python template.pyScript hangs after func1 finishes (or during func2). Minimal repro without computing fib:
python3 -c "
from queue import Queue
from threading import Thread
import time
def worker2(task_q, out):
while True:
try:
task = task_q.get()
except task_q.empty:
return
out.append(task)
q = Queue()
for i in range(3):
q.put((str(i), str(i)))
out = []
threads = [Thread(target=worker2, args=(q, out)) for _ in range(2)]
[t.start() for t in threads]
time.sleep(1)
print('processed', len(out), 'threads still alive', sum(t.is_alive() for t in threads))
"
# Expected if broken: processed 3, threads still alive 2 (process never exits)
| f.write(str_to_write) | ||
|
|
||
|
|
||
| def func2(result_file: str): |
There was a problem hiding this comment.
m7 func2 reads every file in output dir, including result.csv on rerun
Filter to .txt files only (or skip result.csv by name). Right now a second run can treat result.csv as input and corrupt output.
| Create 3 classes with interconnection between them (Student, Teacher, | ||
| Homework) | ||
| Use datetime module for working with date/time | ||
| 1. Homework takes 2 attributes for __init__: tasks text and number of days to complete |
There was a problem hiding this comment.
Homework class does not match the task spec
Spec says deadline should be a timedelta and Homework.init should take text + days. Implementation stores an absolute datetime as deadline and uses a 3-arg constructor. Tests in m3 were written around this shape, so everything passes, but the datetime exercise was skipped.
pytest practice/m3_python_testing/test_task_classes.py -q # passes
python3 -c "
from practice.m2_python_part_2.task_classes import Homework
from datetime import timedelta, datetime
h = Homework('task', timedelta(days=5), datetime.now())
print(type(h.deadline)) # spec expects timedelta; code path via Teacher gives datetime
"
| pass | ||
|
|
||
|
|
||
| def print_name_address(args: argparse.Namespace) -> None: |
There was a problem hiding this comment.
task_4 CLI does not generate NUMBER dicts
Task says python task_4.py NUMBER --field=provider ... and print NUMBER dicts, one per line. Only one dict is printed. number_of_flags is parsed then explicitly skipped in the function, with a comment that it exists only for the test.
Steps to reproduce:
cd practice/m4_python_part_3
python task_4.py 3 --some_name=name --fake-address=address
| for book in books[:3]: | ||
| author_name_list = book["author_name"] | ||
| first_publish_year = book["first_publish_year"] | ||
| title = book["title"] | ||
|
|
||
| print("Book From API:") | ||
| print(f"title: {title}") | ||
| print(f"Auhtor(s): {", ".join(author_name_list)[:-1]}") | ||
| print(f"First publish year: {first_publish_year}") | ||
| print() |
There was a problem hiding this comment.
standalone_api author formatting bug + wrong count
README asks for top 5 results; code uses [:3]. ", ".join(author_name_list)[:-1] strips the last character of the whole string, not the last author. For ["Ada", "Bob"] you get "Ada, Bob"[:-1] -> "Ada, Bo". Also no guard if author_name or first_publish_year is missing (common in Open Library).
Steps to reproduce:
python3 -c "
authors = ['Ada Lovelace', 'Bob Smith']
print(', '.join(authors)[:-1]) # prints: Ada Lovelace, Bob Smit
"
python practice/m6_web_scraping/standalone_api.py
# Compare output count (3 books) vs README requirement (5)
There was a problem hiding this comment.
m6 tests are not part of the main test run
Pytest discovers test_*.py, not tests.py. pytest practice/ runs 34 tests and skips m6 entirely. Rename to test_scraper.py or similar and add deps (beautifulsoup4, requests) to a requirements.txt so CI/reviewers can run them.
pytest practice/ -q
# 34 passed, m6 not mentioned
pytest practice/m6_web_scraping/ -q
# no tests ran (wrong file name)
pip install beautifulsoup4 requests
pytest practice/m6_web_scraping/tests.py -q
# only then tests are found (if deps installed)
| def test_scrap_invalid_catalog_page(): | ||
| scraper = BookScraper() | ||
| with pytest.raises(InvlaidURLScrapingException): | ||
| books_raw = scraper.scrap_multiple_catalog_pages(999, 1000) |
There was a problem hiding this comment.
m6 pagination test hits the real network
This calls the live site twice (pages 999 and 1000) with no mock. Slow, flaky, and not a unit test. Mock requests.get and assert the custom exception on 404.
Steps to reproduce:
pip install beautifulsoup4 requests
pytest practice/m6_web_scraping/tests.py::test_scrap_invalid_catalog_page -v
# Makes real HTTP requests to books.toscrape.com (pages 999 and 1000)
| def scrap_catalog_page(self, page_nr): | ||
| url = self.catalog_page_url_templeate.substitute(page_nr=page_nr) |
There was a problem hiding this comment.
m6 README asks for try/except on HTTP errors; scraper only branches on status codes
README says catch 404, 500, timeouts with try/except so the pipeline does not crash. No timeout= on requests.get, no handling of requests.RequestException, no try/except block. A timeout or connection error will still blow up the whole run.
| print(DATA_DIR) | ||
|
|
||
|
|
||
| def concat_files(data_dir, res_file): |
There was a problem hiding this comment.
Side effects at import time / debug prints left in committed code
Importing or running these modules has side effects (prints, file writes). Debug prints should be under if name == "main": or removed before commit.
Steps to reproduce:
python3 -c "import practice.m2_python_part_2.task_read_write"
# Prints DATA_DIR path and result string; rewrites result.txt
python3 -c "import practice.m2_python_part_2.task_read_write_2"
# Regenerates file1.txt and file2.txt
| temp_max = max(temp_max,int(line)) | ||
| return (temp_min,temp_max) | ||
|
|
||
| print(get_min_max('practice/1_python_part_1/data6.txt')) |
There was a problem hiding this comment.
task6 still points at old directory name
Dirs were renamed to m1_python_part_1 in commit 64df738. This path is stale and breaks if you run the file as-is.
Steps to reproduce:
cd /path/to/repo/root
python practice/m1_python_part_1/task6.py
# FileNotFoundError: practice/1_python_part_1/data6.txt
| res = '' | ||
| for line in lines: | ||
| tab = line.split() | ||
| unique_words = [] | ||
| for word in tab: | ||
| if word not in unique_words: | ||
| unique_words.append(word) | ||
| if len(unique_words) > word_number: | ||
| res+= unique_words[word_number] + ' ' |
There was a problem hiding this comment.
build_from_unique_words returns trailing space
Docstring example is 'b 2 dog', implementation returns 'b 2 dog ' (trailing space). Doctest would catch this.
python3 -c "
from practice.m1_python_part_1.task3 import build_from_unique_words
r = build_from_unique_words('a b c', '1 1 1 2 3', 'cat dog milk', word_number=1)
print(repr(r)) # 'b 2 dog '
print(repr('b 2 dog')) # expected
print(r == 'b 2 dog') # False
"
| print() | ||
|
|
||
|
|
||
| # AI MADE |
There was a problem hiding this comment.
nitpick: Remove the marker. If AI helped with formatting, that is fine, but shouldn't really be left after making sure the code is understood and person committing is willing to be responsible for it. As per IBM manual: machine can never be held accountable, so while devs are and should use AI properly as a tool, they should understand how it influences the scope of their work (said responsibility).
| RES_FILE = CURR_DIR / "result.xml" | ||
|
|
||
|
|
||
| def processCity(data: str) -> str: |
There was a problem hiding this comment.
nitpick: PEP8 and naming (task explicitly asked for it in m2)
processCity / citiesData should be snake_case. standarize -> standardize, acs -> asc. reviewDict -> RATING_MAP or similar. m2 docstring says "PEP8 comply strictly.
also in practice/m6_web_scraping/pipeline.py (lines 15, 18, 31)
| import time | ||
|
|
||
|
|
||
| class InvlaidURLScrapingException(Exception): |
There was a problem hiding this comment.
nitpick: Typos in public names
Invalid, template, standardize. Typos in exception and attribute names are harder to fix later.
|
Minor style issues carried across modules Files: sum = 0def is_http_domain(domain: str) -> bool:
...
pattern = r"^https?://.+\.[a-zA-Z]+/?$"CURR_DIRR = Path(__file__).parentDo not shadow |
|
after running the repo, file1.txt and file 2.txt shows up in git diff |
additionlly pre-commit implemented and run for all files
mainly guarding module-level code with __name__ == __main__
General issues
|
| output_files = [ | ||
| output_dir_path / f"{output_dir_path}" / f"{file_name}_{prefixes[i]}.jsonl" | ||
| for i in range(files_to_create) | ||
| ] |
There was a problem hiding this comment.
This accidentally works for absolute paths (pathlib discards the left side when the next segment is absolute), but for relative paths you get a doubled directory, e.g. ./out → out/out/data_1.jsonl. That can make create_file fail or write files somewhere unexpected. Tests only use resolved absolute paths, so this bug is hidden.
try
output_files = [
output_dir_path / f"{file_name}_{prefixes[i]}.jsonl"
for i in range(files_to_create)
]
| del_count += 1 | ||
| print(f"INFO: Deleted {del_count} files from {output_dir_path}") | ||
| else: | ||
| output_dir_path.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
no check when path exists but is not a directory (this + capstone/arguments_cleaning.py:108-113
As per spec: "If path exist and it is not a directory - exit with error log." If /tmp/foo.txt already exists as a file, mkdir raises FileExistsError and the user sees a full traceback — opposite of the assignment rules.
Console utilities should fail gracefully with a clear message and sys.exit(1).
| if not output_path.parent.exists(): | ||
| raise OutputDirectoryNotPrepared( | ||
| "You probably forgot to run prepare_output_directory function before creating files" |
There was a problem hiding this comment.
raise in worker process
Spec explicitly says not to use raise for errors that should exit the utility. In multiprocessing this prints a traceback from the worker and is only caught generically in perform_file_generation — the main process still exits 0.
Log the error, return a failure indicator, and have the orchestrator call sys.exit(1) if any worker fails — or validate paths before spawning workers so this branch is unreachable.
| except Exception as e: | ||
| print(f"ERROR: Worker crashed with error: {e}", file=sys.stderr) |
There was a problem hiding this comment.
worker failure does not exit with error code
Worker crash is printed but the program continues and exits successfully. Spec: errors → logging.error → exit.
more like
logging.error("Worker crashed: %s", e)
sys.exit(1)
| data_schema: Dict[str, str] | ||
| data_lines: int = Field(ge=0) | ||
| clear_path: bool = False | ||
| multiprocessing: int = Field(g=0) |
There was a problem hiding this comment.
typo Field(g=0) instead of Field(ge=1)
g is not a valid Pydantic constraint (pytest emits PydanticDeprecatedSince20 warning; the g is ignored). Validation only happens in @field_validator("multiprocessing"), so behavior works today, but the model declaration is misleading and will break on Pydantic v3.
| if item.is_file() and item.name.startswith(file_name): | ||
| item.unlink() | ||
| del_count += 1 | ||
| print(f"INFO: Deleted {del_count} files from {output_dir_path}") |
There was a problem hiding this comment.
logging use print, not logging
Assignment wants logging for all steps. INFO: string prefix mimics logging but bypasses the required module.
Configure once at startup (logging.basicConfig(level=logging.INFO, format="%(levelname)s: %(message)s")) and replace print("INFO: ...") / stderr prints for errors with logging.info / logging.error.
In general also the convention is that if we are working with multiple modules/files, logging is much better for tracing wth is happening.
Additionally: check out loguru (overkill for this project but extremely cool logging setup module )
There was a problem hiding this comment.
also in a lot of other places errors and infos use print, not logging.error, please take a look to use logging rather than print
| case "uuid": | ||
| prefixes = [str(uuid.uuid4()) for _ in range(files_to_create)] | ||
| output_files = [ | ||
| output_dir_path / f"{output_dir_path}" / f"{file_name}_{prefixes[i]}.jsonl" |
There was a problem hiding this comment.
single-file naming does not match spec
Spec: one file → file_name.json (no prefix). Implementation always uses file_name_{prefix}.jsonl, so files_count=1 produces export_1.jsonl instead of export.jsonl.
Verified manually: --files_count 1 --file_name single → single_1.jsonl.
should be if-else
| import sys | ||
| import time | ||
| import uuid | ||
| from concurrent.futures import ProcessPoolExecutor, as_completed |
There was a problem hiding this comment.
Required libraries list includes multiprocessing. ProcessPoolExecutor is a valid modern choice but graders may check imports.
as per spec I'd try to follow it closely as it might be what the professor would be looking for
|
|
||
|
|
||
| # Console Worker (not worker anymore but lets keep it that way) | ||
| def print_schema_to_console(data_schema: Dict[str, str], num_of_lines: int) -> str: |
There was a problem hiding this comment.
no test for files_count == 0 (console output mode). Spec requires this behavior explicitly. Implementation exists (print_schema_to_console) but isn't tested.
Manual run works; automated coverage would catch regressions
| ] | ||
|
|
||
|
|
||
| def test_perform_file_generation(temp_output_dir, clean_args_factory): |
There was a problem hiding this comment.
Checks file count, not data_lines per file. Spec: "each file must have a number of lines == data_lines".
|
|
||
| orchestrate_data_generation(clean_args=args_clean) | ||
|
|
||
| # TODO somehow make it work with provided dataschemas |
2 participants
Do not merge, it's for review only.
General suggestion (optional, good to know):
Minimal copy-paste setup so trailing whitespace, basic PEP8, and formatting get caught before commit. Uses pre-commit (runs checks on
git commit) and Ruff (lint + format, replaces flake8/black/isort for most cases).1. Install
2. Create
.pre-commit-config.yamlin repo root3. Create
pyproject.tomlin repo root4. Enable and run
pre-commit installwires hooks into git.run --all-fileschecks the whole repo once (first run will report a lot; that is normal).What this catches
Note to Mateusz
A lot of the review items above (trailing spaces,
processCityvsprocess_city,sumshadowing builtin, strayprint()at module level) are exactly what Ruff flags. You do not need to memorize every PEP8 rule; run the hooks, read the message, fix or ask.Start with
pre-commit run --all-fileson a clean branch and fix file by file. Do not--no-verifyyour way through every commit; that defeats the point.ruff check --fixandruff formatdo the same checks outside git if you want to fix before committing:Hooks auto-fix some things (trailing whitespace, formatting). Others you fix by hand (wrong logic, missing mocks in tests). Lint will not catch bugs like the m7
func2deadlock; tests and actually running the script still matter.Optional later: add
pytestto CI or pre-commit once the suite is green and fast enough. For now lint + format is enough.