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:
convertfunctions 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 typelog_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 Listimport humanfriendlyis it added to pyproject.toml?
now added
@ -0,0 +33,4 @@return conf_stagedef get_executorWithConfig(underscore
fixed
@ -0,0 +1,33 @@*.avi filter=lfs diff=lfs merge=lfs -textare 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"]] = Nonediff: Optional[ParserDiff] = ParserDiff()class Config:deprecated
@bomingzh any suggestions on the structure?
fixed with
model_configstr here need to be a
StrEnumnow.But I guess we don't know the set of case in advance, making it dynamic
StrEnumis meaninglessline changed, the comment is for
parsersAlso, remove the compilation stage in test cases.
@bomingzh compilation stage in testcases are removed
@ -1,37 +1,21 @@import jsonthis 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 -textwhy here?
you mean the existence of the file? I design the logic of
immutable foldersto be like this:Why does this test need immutable files?
you are right
fixed
@ -21,0 +45,4 @@# Construct healthcheck stageif (not repo_conf.force_skip_heatlh_check_on_test# or os.environ.get("PYTEST_CURRENT_TEST") is Nonewhy?
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-commiton 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
timeparseandhumanfriendlywould deal wth that?resolved.
@ -31,2 +98,3 @@class Release(BaseModel):deadline: Optional[datetime] # RFC 3339 formatted date-time with offsetdeadline: Optional[datetime] = None # RFC 3339 formatted date-time with offsetbegin_time: Optional[datetime] = Nonebegin_timeandend_timeto 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_000is it in the correct unit?
Give all the fields in
Limita 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:continuemove
continueto the other branch to reduce nestingI mean
fixed.
@ -0,0 +172,4 @@show_files = []if (task_stage.result_detail.stdoutand task_stage.result_detail.stdout is not Nonejust
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
pyclnit 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
pyclnrepo.@ -0,0 +5,4 @@def get_joj1_run_stage(joj1_config: joj1.Config) -> task.Stage:default_cpu = timeparse("1s")just use
humanfriendly.parse_timespanseems outdated, mark as resolved.
@ -1,40 +1,121 @@from datetime import datetimeevery 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 stageif 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
cachedwill 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.
settry it yourself
I see why
resolved.
@ -0,0 +1,232 @@import rePath should not be relative to
JOJ3_CONFIG_ROOTin this file, should be relative totask.tomldirI reckon you said things is relative to
JOJ3_CONFIG_ROOTin JTC before. we have atask.typeintask.tomlto mend the pathconfig.pathis 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.pathis relative toJOJ3_CONFIG_ROOT, sotask.tomlwill located atJOJ3_CONFIG_ROOT / task_conf.pathin 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 outdatedgroup=(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 situationWhat 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 reimport shlexSome
with_.updateis still using raw dict, not model withmodel_dump.@ -0,0 +31,4 @@]),)fix_result_detail(task_stage, conf_stage)should loop through
conf_stage.parsershere and update thewithfield according to the parser name.I think its already implemented in each of the
fix_parsersfunctionsNo, do not find the parser in the
fix_xxxfunction. 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: strgroup: strgroup: 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.ParserConfigThe 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 applicationif 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_configIs 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.parentasbase_dirin 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_casesJust pass
conf_stage.executorto 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 = 65is it used?
testcases in basic is intend to show a comprehensive situation.
if unused, leave a comment
Then why is there a
stdoutfield inclass Limit@bomingzh in toml now should we accept both
32mand32or just32mnow?I reckon only
32mwould be goodyes, just str
@ -156,0 +168,4 @@class DummyConfig(BaseModel):score: Optional[int] = NoneAre these
Optionalnecessary?@ -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 = 0parsers: List[str] = [] # list of parsersthis should be the
StrEnumyes
It is supported now.
@bomingzh
we probably need to update some dependencies, everything works fine on my local sides, seems
StrEnumare supported frompython3.11No, just inherit from
(str, Enum).OK
@ -0,0 +3,4 @@import humanfriendly# FIXME: we don't need to compact for intwhat 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 hardcodedgroup_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, taskfrom joj3_config_generator.models.common import Memory, Timefrom joj3_config_generator.models.const import JOJ3_CONFIG_ROOTfrom joj3_config_generator.models.task import Parser as parser_enumParserEnum@ -156,0 +210,4 @@outputs: List[DiffOutputConfig] = []# to get minimum flexibility, may refine in futurewhat 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 defineddiff_output = (case_stage.diff.outputif case_stage.diff and case_stage.diff.outputwill 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 casesis 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