dev #10
No reviewers
Labels
No Label
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: JOJ/JOJ3-config-generator#10
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "dev"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What I have done:
convert
functions is mature, tested in engr151-24fa last two homeworks and engr151-24fa p3TODO
@bomingzh Could you please help me do a code review first then I shall fix all things and rebase? I checked the source code of JOJ3 currently, and find we have more parsers, such as plugin and tierscore, I haven''t added them yet, and some of the keys might have changed such as clang-tidy, can you help me take a look if current version is still up to date? I ll redo the fix tomorrow
The test cases should be split into smaller parts. Each test case tests one parser.
@ -16,3 +16,3 @@
eggs/
.eggs/
lib/
# lib/
should be ignored
fixed
@ -11,1 +30,3 @@
log_path=f"{task_conf.task.replace(' ', '_')}.log",
name=task_conf.task.name,
# exact folder difference specified by type
log_path=f"{Path.home()}/.cache/joj3/{task_conf.task.type_}.log",
Make this
Path.home()
default to/home/tt
. For now, create a const for this dir.fixed
@ -0,0 +1,52 @@
from typing import List
import humanfriendly
is it added to pyproject.toml?
now added
@ -0,0 +33,4 @@
return conf_stage
def get_executorWithConfig(
underscore
fixed
@ -0,0 +1,33 @@
*.avi filter=lfs diff=lfs merge=lfs -text
are these files ready to be tested? or are they just examples?
they are just examples
@bomingzh smaller testcases are ready, each one with single parsers
@ -28,0 +82,4 @@
cases: Optional[Dict[str, "Stage"]] = None
diff: Optional[ParserDiff] = ParserDiff()
class Config:
deprecated
@bomingzh any suggestions on the structure?
fixed with
model_config
str here need to be a
StrEnum
now.But I guess we don't know the set of case in advance, making it dynamic
StrEnum
is meaninglessline changed, the comment is for
parsers
Also, remove the compilation stage in test cases.
@bomingzh compilation stage in testcases are removed
@ -1,37 +1,21 @@
import json
this file should not be changed
@bomingzh code refactor has been done.
Are all the old conversations resolved?
Yes, and I just fixed another path problems about
diff
, but turns out it cannot pass test now 😂 am looking into it.Then click the "Resolve conversation" button.
Rebase is needed, some codes are not from latest master.
I merged master here #11, and double checked, should all be fine I guess? Do you have more suggestions on the code and adding features about parsers?
@ -0,0 +1,33 @@
*.avi filter=lfs diff=lfs merge=lfs -text
why here?
you mean the existence of the file? I design the logic of
immutable folders
to be like this:Why does this test need immutable files?
you are right
fixed
@ -21,0 +45,4 @@
# Construct healthcheck stage
if (
not repo_conf.force_skip_heatlh_check_on_test
# or os.environ.get("PYTEST_CURRENT_TEST") is None
why?
forgot to uncommented 😭
fixed
@ -59,6 +59,7 @@ def convert(
Convert given dir of JOJ3 toml config files to JOJ3 json config files
"""
logger.info(f"Converting files in {root.absolute()}")
root = root.absolute()
why?
fixed
@ -25,0 +21,4 @@
stdin: Optional[CmdFile] = CmdFile(content="")
stdout: Optional[CmdFile] = CmdFile(name="stdout", max=4 * 1024)
stderr: Optional[CmdFile] = CmdFile(name="stderr", max=4 * 1024)
cpu_limit: int = Field(4 * 1000000000000, serialization_alias="cpuLimit")
default value too large, should be 1s 128MB
will fix later, we need to hold some test I reckon?
no
@bomingzh fixed
Tests not passed
@bomingzh test can pass now, with conflict with master fixed.
Is
pre-commit
on your local dev env installed correctly?@ -25,0 +27,4 @@
stderr: Optional[CmdFile] = CmdFile(
name="stderr", max=humanfriendly.parse_size("128m")
)
cpu_limit: int = Field(timeparse("1s"), serialization_alias="cpuLimit")
unit?
I think
timeparse
andhumanfriendly
would deal wth that?resolved.
@ -31,2 +98,3 @@
class Release(BaseModel):
deadline: Optional[datetime] # RFC 3339 formatted date-time with offset
deadline: Optional[datetime] = None # RFC 3339 formatted date-time with offset
begin_time: Optional[datetime] = None
begin_time
andend_time
to make them matchresolved
@ -0,0 +7,4 @@
def get_grading_repo_name() -> str:
# FIXME: uncomment back when everything is ready!
host_name = "ece280"
make it a field in repo.toml, if it is unset, then use
socket.gethostname
. We set this value to pass the test.resolved.
@ -0,0 +51,4 @@
immutable_files = immutable_files + name + " "
else:
immutable_files = immutable_files + name + ","
chore = "/usr/local/bin/repo-health-checker -root=. "
why not
args = "hc -root=. "
resolved.
@ -0,0 +77,4 @@
copy_in_cached={file: file for file in cached},
copy_out_cached=file_export if file_export is not None else [],
cpu_limit=(
task_stage.limit.cpu * 1_000_000_000_000
is it in the correct unit?
Give all the fields in
Limit
a default value and make it non optional in task_stage.limitresolved
@ -0,0 +126,4 @@
) -> result.StageDetail:
keyword_parser = ["clangtidy", "keyword", "cppcheck", "cpplint"]
if task_stage.parsers is not None:
for parser in task_stage.parsers:
->
resolved.
@ -0,0 +152,4 @@
{
"matches": keyword_weight,
"fullscore": 0,
"minscore": -1000,
these fields do not exist now
resolved
@ -0,0 +157,4 @@
}
)
else:
continue
move
continue
to the other branch to reduce nestingI mean
fixed.
@ -0,0 +172,4 @@
show_files = []
if (
task_stage.result_detail.stdout
and task_stage.result_detail.stdout is not None
just
if task_stage.result_detail.stdout:
@ -0,0 +208,4 @@
if (
getattr(task_stage, parser.replace("-", "_"), None) is not None
) and (task_stage.result_status is not None):
dummy_parser_.with_.update(
Create models for these dicts, then update them with the dict from
model_dump
@bomingzh I don't think we can change it. This is to make proper alias so that we can get the content of
result-status
.Then
dummy_parser_.with_.update(dummy_config(...).model_dump())
.fixed
for
pycln
it has some path issue so I skip it and runpycln .
locally to do the test (sometimes I may forgot...)You can create an issue in
pycln
repo.@ -0,0 +5,4 @@
def get_joj1_run_stage(joj1_config: joj1.Config) -> task.Stage:
default_cpu = timeparse("1s")
just use
humanfriendly.parse_timespan
seems outdated, mark as resolved.
@ -1,40 +1,121 @@
from datetime import datetime
every field in this file should not be optional. we give an default value here if any field does not exist
and use underscore naming in this file
fixed
fixed.
@ -21,0 +34,4 @@
# Construct healthcheck stage
if not repo_conf.force_skip_health_check_on_test or not current_test:
result_conf.stage.stages.append(get_healthcheck_config(repo_conf))
cached: List[str] = []
where is it used?
this should be storing all the files that are about to be copy in or out
It is as the input and output for the following functions about parsers
so this feature is not implemented?
it is
this is a loop, so this
cached
will be updated in every round of stageThe return value is unnecessary.
I have a lazing coding style here, everything has get imported would get exported, so should maintain this until the end of the loop. Everything is exported in previous stage would be imported in the next stage.
set
try it yourself
I see why
resolved.
@ -0,0 +1,232 @@
import re
Path should not be relative to
JOJ3_CONFIG_ROOT
in this file, should be relative totask.toml
dirI reckon you said things is relative to
JOJ3_CONFIG_ROOT
in JTC before. we have atask.type
intask.toml
to mend the pathconfig.path
is relative toJOJ3_CONFIG_ROOT
.could you explain further? I m not quite sure my understanding is clear.
In
joj3_config_generator/models/task.py
,Config.path
is relative toJOJ3_CONFIG_ROOT
, sotask.toml
will located atJOJ3_CONFIG_ROOT / task_conf.path
in JTC.@ -0,0 +13,4 @@
name=task_stage.name,
# group is determined by adding between "[]" in the name of the task
# FIXME: this is probably outdated
group=(
BTW, is this outdated?
Never heard about this rule.
@manuel what would be the current intended rule for
group
?seems current strategy is fine, resolved.
@ -54,3 +59,3 @@
copy_in_cached: Dict[str, str] = Field({}, serialization_alias="copyInCached")
copy_in_dir: str = Field(".", serialization_alias="copyInDir")
copy_out: List[str] = Field([], serialization_alias="copyOut")
# reconsider this default situation
What is the conclusion?
should be already solved.
@ -0,0 +73,4 @@
if parser in keyword_parser:
keyword_parser_ = next(p for p in conf_stage.parsers if p.name == parser)
keyword_weight = []
if getattr(task_stage, parser, None) is not None:
do not use
getattr
, visit the field explictlyresolved.
@ -0,0 +60,4 @@
cases=[],
)
for file in file_export:
if file not in cached:
not necessary
resolved.
@ -0,0 +1,232 @@
import re
import shlex
Some
with_.update
is still using raw dict, not model withmodel_dump
.@ -0,0 +31,4 @@
]
),
)
fix_result_detail(task_stage, conf_stage)
should loop through
conf_stage.parsers
here and update thewith
field according to the parser name.I think its already implemented in each of the
fix_parsers
functionsNo, do not find the parser in the
fix_xxx
function. Instead, iterate through the parsers here and decide how to fill in thewith
.resolved.
Use a dict to store parser name, field, function to process.
resolved.
@ -118,3 +122,3 @@
class StageDetail(BaseModel):
name: str
group: str
group: Optional[str] = ""
Should it be optional?
@ -0,0 +32,4 @@
)
processed_dict = get_processed_dict(task_stage)
for idx, parser in enumerate(task_stage.parsers):
if parser in processed_dict or parser.replace("-", "_") in processed_dict:
Do we need to support both kinds of names?
probably yes, since it is easy for new ta to type it wrong
parsers name should be a str enum, force them to use the correct names
ok, then removed.
@ -0,0 +91,4 @@
def fix_keyword(
keyword_config: task.ParserKeyword, keyword_parser_: result.ParserConfig
The reason for the suffix in
keyword_parser_
?just forgot to remove, sorry
@ -0,0 +137,4 @@
dummy_parser_config: task.ParserDummy, dummy_parser: result.ParserConfig
) -> None:
# we don't use dummy parser in real application
if dummy_parser_config is None:
When will it be None?
@ -0,0 +163,4 @@
diff_parser_config: result.ParserConfig,
conf_stage: result.StageDetail,
) -> None:
diff_parser = diff_parser_config
Is it necessary to rename?
@ -0,0 +186,4 @@
stage_cases.append(
result.OptionalCmd(
stdin=result.LocalFile(
src=str(JOJ3_CONFIG_ROOT / task_conf.path.parent / stdin),
Pass
JOJ3_CONFIG_ROOT / task_conf.path.parent
asbase_dir
in parameters.resolved.
@ -0,0 +220,4 @@
)
)
if diff_parser:
When will it be None?
@ -0,0 +224,4 @@
diff_parser.with_.update(
result.DiffConfig(name="diff", cases=parser_cases).model_dump(by_alias=True)
)
conf_stage.executor.with_.cases = stage_cases
Just pass
conf_stage.executor
to this function rather then the wholeconf_stage
.@ -31,0 +64,4 @@
[[stages]]
name = "[cq] Cpplint"
command = "cpplint --linelength=120 --filter=-legal,-readability/casting,-whitespace,-runtime/printf,-runtime/threadsafe_fn,-runtime/int,-readability/todo,-build/include_subdir,-build/header_guard,-build/include_what_you_use --recursive --exclude=build h7/ex2.cpp"
limit.stdout = 65
is it used?
testcases in basic is intend to show a comprehensive situation.
if unused, leave a comment
Then why is there a
stdout
field inclass Limit
@bomingzh in toml now should we accept both
32m
and32
or just32m
now?I reckon only
32m
would be goodyes, just str
@ -156,0 +168,4 @@
class DummyConfig(BaseModel):
score: Optional[int] = None
Are these
Optional
necessary?@ -156,0 +175,4 @@
)
class DiffOutputConfig(BaseModel):
also these
Optional
?I guess some of the field have default within JOJ3, so I choose optional just to reduce the length of json previously
better put all defaults here then we only need to check the code here
indeed.
@ -29,0 +84,4 @@
in_: str = Field("", alias="in")
out_: str = Field("", alias="out")
score: int = 0
parsers: List[str] = [] # list of parsers
this should be the
StrEnum
yes
It is supported now.
@bomingzh
we probably need to update some dependencies, everything works fine on my local sides, seems
StrEnum
are supported frompython3.11
No, just inherit from
(str, Enum)
.OK
@ -0,0 +3,4 @@
import humanfriendly
# FIXME: we don't need to compact for int
what compact?
no int input, should be done already, so it can be removed. I added that several commit before before you removed that 😇
@ -0,0 +55,4 @@
+ f"{repo_conf.groups.time_period_hour[i]},"
)
# default value hardcoded
group_config = group_config + "=100:24"
do we need that default?
what is the situation internally in JOJ3? I just added that blindly when I reached that stage. Overall I think 100 times per day should be a reasonable and safe default? considering that there may be group works and due date
They can manually add it if needed. Or just move it to the model default.
removed.
@ -0,0 +6,4 @@
from joj3_config_generator.models import result, task
from joj3_config_generator.models.common import Memory, Time
from joj3_config_generator.models.const import JOJ3_CONFIG_ROOT
from joj3_config_generator.models.task import Parser as parser_enum
ParserEnum
@ -156,0 +210,4 @@
outputs: List[DiffOutputConfig] = []
# to get minimum flexibility, may refine in future
what needs to be refined?
just feels bit waste since this outputs only have one field before I coded the logic, I wrote that comment without checking the details of diff parser. But it should be the best strategy to show the structures as well as give flexiblity for extension now.
@ -0,0 +190,4 @@
# Ensure case_stage.diff and case_stage.diff.output are defined
diff_output = (
case_stage.diff.output
if case_stage.diff and case_stage.diff.output
will it be None now?
nope 😂 bad coding strategy before
@ -0,0 +76,4 @@
copy_in={
file: result.LocalFile(src=str(JOJ3_CONFIG_ROOT / file))
# all copyin files store in this tools folder
# are there any corner cases
is there a conclusion now? or it should have a prefix
TODO:
?so far works fine on 280 sides, indicating that pbs in 151 can be resolved with proper guidelines I think.
ok, then add that prefix
WIP: devto dev